Skip to content

fix(storage): bind context-offload limits to the first concurrent opener - #5810

Open
Aaron-Ben wants to merge 1 commit into
apache:mainfrom
Aaron-Ben:fix/context-offload-single-flight-race
Open

Aaron-Ben wants to merge 1 commit into
apache:mainfrom
Aaron-Ben:fix/context-offload-single-flight-race

Conversation

@Aaron-Ben

Copy link
Copy Markdown

Summary

openInteractiveContextOffloadStoreForWrite awaited the on-disk lease check before it claimed or joined the per-lease single flight. Concurrent callers finish that check in no fixed order, so a later caller with different limits could become the writer and reject the earlier callers with "different limits". This made single-flights one limit-bound writer and snapshots admitted inputs flaky.

The opener now validates the lease synchronously (assertStorageRootLeaseActive) and claims or joins the slot before its first await, so the earliest caller always binds the limits. The root identity is still verified on disk before a writer is returned:

  • a new writer is opened through runWithStorageRootLease, which checks the root identity, and is re-checked afterwards (unchanged);
  • a caller joining an in-flight open shares that open's checks;
  • a caller waiting on a close re-enters the opener;
  • a caller reusing an existing writer now awaits assertStorageRootLease explicitly, and re-enters if the writer was closed during that check.

The other storage authorities use the same single-flight pattern but take no limits, so every caller gets the same writer whichever call wins; only this store needed the change.

Fixes #5801

Verification

  • New test binds the first caller limits whatever order concurrent lease checks finish in replays the race 500 times (~0.4 s). Without the fix it failed 10/10 runs in isolation and 9/10 runs as part of the file; with the fix the file passed 30/30 runs.
  • The issue's race scenario (two opens, one with conflicting limits) was replayed 2,100 times: 1–3% of rounds went wrong before the fix, 0 after.
  • @maka/storage build (tsc) and full dist suite: 1548 tests, 0 failures (Node 24.15.0, macOS arm64).
  • biome check on the changed files: clean.
  • Not run: the full monorepo npm test.

Behavior change

Concurrent openers are now admitted in call order. One minor difference in error precedence: when a writer already exists, a limits mismatch is now reported before the on-disk root identity check, so a caller with both a relocated root and mismatched limits sees "different limits" rather than the lease error.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code analysed the race, wrote the fix and tests, and ran the verification; I reviewed and directed the change. The commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Concurrent openInteractiveContextOffloadStoreForWrite calls each awaited
the on-disk lease check before claiming or joining the per-lease single
flight. Those checks finish in no fixed order, so a later caller with
different limits could become the writer and reject the earlier callers.

Check the lease synchronously and claim or join the slot before the first
await; the root identity is still verified on disk before a writer is
returned. The single-flight test now awaits the conflicting open with the
others, and a new test replays the race to guard the ordering.

Fixes apache#5801

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 29, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change claims the per-lease context-offload writer slot before the first await, so the first valid caller's snapshotted limits win even if concurrent on-disk root-identity checks complete in a different order. The opening path still validates the lease and root identity before constructing and returning the SQLite-backed writer; an existing writer is revalidated and rechecked after the await. I found no actionable issue in these paths.

I reviewed the open/join/close paths and the new concurrent-limits regression. On Node 24, the core and storage builds and all five focused context-offload-store tests pass, including the 500-round concurrent-open test. The change has no schema migration; a fresh-main merge-tree and diff check are clean. Only the label hosted check is visible on this head, so I cannot treat the full CI suite or packaged Host/Desktop behavior as verified. This is not merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(storage): context-offload single-flight test is flaky when the conflicting open wins the race

2 participants