Fix/ocisdev 1433 by 2403905 · Pull Request #748 · owncloud/reva

2403905 · 2026-09-25T10:28:13Z

No description provided.

kw-security · 2026-09-25T10:28:28Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

kobergj

Reviewed the public-share manager changes (d4f339c + 95d977d). The direction is right - taking the read-modify-write out of GetPublicShare / GetPublicShareByToken / ListPublicShares, dropping the manager lock before the gateway Stat fan-out, and deleting the signal.Notify(SIGHUP, SIGINT, SIGQUIT) janitor hack (which was suppressing the default signal disposition process-wide) are all clear improvements. Wiring Close through rgrpc.cleanupServices and passing the request context into init() are good too.

One blocking issue in cs3.Write (cache/mtime inconsistency that defeats IfUnmodifiedSince), plus a few smaller things inline.

One process note: no changelog entry for these two commits, although the other commits in this PR each add one - and this change is user-visible (expired shares are no longer purged on access, janitor cadence changed, new shutdown semantics).

kobergj · 2026-09-25T13:35:41Z

+	// independent copy (see persistence.Copy), it has to be done explicitly
+	// here, or the cache would only pick up our own write once some later
+	// external write advances the remote mtime past our stale one.
+	if info, statErr := p.s.Stat(ctx, "publicshares.json"); statErr == nil {


Blocking. Pairing our content with an mtime from a separate Stat after the upload makes the cache self-inconsistent, and that defeats the IfUnmodifiedSince guard on line 139.

Before this change, p.db.mtime was only ever assigned in Read, right next to the content downloaded at that mtime - they were consistent by construction. With two instances writing the same publicshares.json:

A: Read -> cache mtime M0

A: Upload(C_A, IfUnmodifiedSince: M0) -> OK, remote becomes M1

B, in the window between A's PUT and A's Stat: Read -> M1, Upload(C_B, IfUnmodifiedSince: M1) -> OK, remote becomes M2/C_B

A: Stat -> M2. A now caches mtime M2 with content C_A

A's next Read: M2.After(M2) == false -> no refetch -> A keeps serving C_A, so B's share is invisible on A indefinitely

A's next Write: IfUnmodifiedSince: M2, remote mtime is M2, After is false -> precondition passes -> A uploads C_A + its mutation, silently dropping B's share

UploadResponse only carries Etag/FileID, so there is no mtime to reuse. Two ways out:

Invalidate instead of syncing: on success set p.db.mtime = time.Time{} and clear the cached content, so the next Read unconditionally refetches. One extra download per write, obviously correct.

Or set MTime on the UploadRequest (the metadata storage supports it via X-OC-Mtime) and cache that value. Content and mtime stay consistent, and it removes this Stat - a full round trip currently held under p.mu.

Either way, statErr should not be swallowed silently.

// Invalidate the cache to minimize the risk of inconsistency. p.db.mtime = time.Time{} p.db.publicShares = persistence.PublicShares{}

kobergj · 2026-09-25T13:35:41Z

-	for _, v := range db {
+	var changed bool
+	for id, v := range db {
 		d := v.(map[string]interface{})["share"]


These two assertions still panic: v.(map[string]interface{}) and d.(string). This runs in a bare goroutine with no recover(), so one malformed entry in publicshares.json takes the whole service down - and repeats every janitor tick.

The continue added just below shows the intent was to tolerate bad entries, so it seems worth extending it one line up, especially since OCISDEV-877 in this same PR exists because partially-broken entries do occur:

d, ok := v.(map[string]interface{})["share"].(string) if !ok { continue } var ps link.PublicShare if err := utils.UnmarshalJSONToProtoV1([]byte(d), &ps); err != nil { continue }

The json.go has about 10 v.(map[string]interface{})... unsafe type assertions. This particular safe-assertion fix cannot resolve all of them. We should update all of them or keep them as is.

kobergj · 2026-09-25T13:35:41Z

+		return err
+	}
+
 	m.mutex.Lock()


This holds the exclusive manager lock across a persistence Read and Write - two metadata-storage round trips, bounded at 60s - blocking every public-share read in the process for the duration.

Much better than the previous N sequential read+write cycles under the lock, so not blocking. But for a change whose point is read-path contention, the janitor is now the remaining stop-the-world point. Worth considering: collect the expired IDs under RLock, then take the write lock only for the read-modify-write, and let IfUnmodifiedSince + a retry handle the race.

We will not use RLock and Lock together here because threre is no database consistency mechanism no the db level. We can overwrite the new public link created between these two requests.

kobergj · 2026-09-25T13:35:41Z

+// that a caller which keeps reading the result after releasing its lock
+// cannot race a writer that later mutates an existing share's fields in
+// place (see manager.UpdatePublicShare).
+func Copy(db PublicShares) PublicShares {


This now runs on every read path, so each ListPublicShares / GetPublicShareByToken allocates a fresh copy of the entire share database (N+1 maps). That is a real cost on instances with a large publicshares.json, which are exactly the ones hurting from read-path contention today.

Note that the cs3 cache is already effectively immutable once published: both the refill in Read and Write replace p.db.publicShares with a fresh map rather than mutating it in place. The copy is only needed because the manager's write paths mutate what Read handed them - UpdatePublicShare doing data["share"] = ..., and delete(db, id) in revokePublicShare / cleanupExpiredShares.

So Copy could move into those four write paths and Read could document its result as read-only. Same race fix, allocation-free reads.

Moved the database copy from the persist label to the manager level only for create and update operations.

kobergj · 2026-09-25T13:35:41Z

 	}
 	if c.JanitorRunInterval == 0 {
-		c.JanitorRunInterval = 60
+		c.JanitorRunInterval = 600


60 -> 600 is a 10x change to the default cleanup cadence, and it is not mentioned in the commit message or a changelog. Intentional? It is defensible now that the read paths no longer purge, but on a stable branch it should be deliberate and documented.

That is only an intermediate fix. In the future, we need to trigger the janitor from a CLI command.

kobergj · 2026-09-25T13:35:41Z

+
+	const writers = 8
+	const readers = 8
+	const iterations = 500


Nit: 8 writers x 500 iterations is 4000 real disk writes plus 4000 reads - ~15s locally without -race, and noticeably worse in CI with it. ~50 iterations pins the same regression.

kobergj · 2026-09-25T13:35:41Z

+				// Overlapping reads should finish close to a single delay - allow
+				// generous slack for scheduling noise without letting a real
+				// regression pass.
+				Expect(time.Since(start)).To(BeNumerically("<", delay*3))


Nit: this asserts on a wall-clock ratio (< 450ms vs 1.2s serialized), which will be flaky on a loaded shared runner. The property being tested is the right one - a counter in slowReadPersistence.Read tracking max observed concurrency (Expect(maxConcurrent).To(BeNumerically(">", 1))) would test it deterministically.

kobergj · 2026-09-25T13:35:41Z

+
+	// Give the Read goroutine time to be inside its (slow) Stat call,
+	// holding mu, before Init races it.
+	time.Sleep(delay / 5)


Nit: time.Sleep(delay / 5) to get the other goroutine inside its Stat, then asserting < delay/2, is the same wall-clock flakiness as the ginkgo test in json_test.go. Signalling from inside the fake Stat (a channel the storage closes on entry) removes the guess.

kobergj · 2026-09-25T13:35:41Z

 	return m, nil
 }

+var _ publicshare.ClosableManager = (*manager)(nil)


Nit: this interface assertion reads better next to the manager type declaration than between New and commonConfig.

…n a public shares

The cs3 metadata storage authenticates as a system user before every operation, and that Authenticate call carried no deadline. A gateway that accepted the connection but never answered it - because it was itself waiting on a stalled storage provider - parked the calling goroutine for the lifetime of the process, with no log output at all. The call now runs on its own bounded context and logs a warning when the deadline is exhausted. The context returned to the caller deliberately keeps no deadline: callers run their own RPC on it after getAuthContext returns. Adds the first tests for this package, including one that reproduces the unbounded wait against an in-process gateway that never answers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The grpc client connections of the pool were created without keepalive parameters, so a peer that stopped answering on an established connection - a black-holed node, a wedged process - was indistinguishable from a peer that was merely slow, and a request without a deadline waited for the lifetime of the process. The clients now ping the peer while a request is in flight and fail the requests on a connection that does not answer, tunable with GRPC_CLIENT_KEEPALIVE_TIME and GRPC_CLIENT_KEEPALIVE_TIMEOUT. The servers got the matching enforcement policy so they accept those pings. GRPC_MAX_CONNECTION_AGE is removed along with it. It closed healthy connections on a timer, never ended a request that was already in flight because the grace period was left at infinity, and fell back to doing nothing at all whenever its value had no unit suffix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…FIND requests.

…sist label to the manager.

kobergj requested changes Sep 25, 2026

View reviewed changes

2403905 marked this pull request as ready for review September 29, 2026 07:18

2403905 requested a review from kobergj September 29, 2026 07:18

2403905 force-pushed the fix/OCISDEV-1433 branch from e2e5895 to 6760373 Compare September 29, 2026 10:07

2403905 and others added 9 commits September 29, 2026 12:14

fix: Fixed the revok during the least/read operations and the mutex i…

891754f

…n a public shares

fix: fixed the lazy load mutex

35dac22

move resolveLinkshares to the dedicated function

2977904

rewok the default keepalive.ClientParameters

2aa04f7

update the public share manager

7e38d54

fix: Disable the resolution of public link shares when answering PROP…

85adf4c

…FIND requests.

fix: Invalidate the cache after write. Moved the db copy from the per…

3755f25

…sist label to the manager.

2403905 force-pushed the fix/OCISDEV-1433 branch from 6760373 to 3755f25 Compare September 29, 2026 10:14

fix: fix unsafe type assertions, to avoid the panic

0bec0b9

2403905 force-pushed the fix/OCISDEV-1433 branch from dad8d79 to 0bec0b9 Compare September 29, 2026 14:36

fix: The keepalive grpc clinet configuration updated

e6768c6

Conversation

2403905 commented Sep 25, 2026

Uh oh!

kw-security commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

kobergj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2403905 Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2403905 Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kw-security commented Sep 25, 2026 •

edited

Loading

2403905 Sep 28, 2026 •

edited

Loading

2403905 Sep 28, 2026 •

edited

Loading