Skip to content

fix(storage): close usage stores after lease revocation - #2365

Merged
Astro-Han merged 2 commits into
apache:mainfrom
liugddx:fix/windows-usage-stores-cleanup
Aug 7, 2026
Merged

fix(storage): close usage stores after lease revocation#2365
Astro-Han merged 2 commits into
apache:mainfrom
liugddx:fix/windows-usage-stores-cleanup

Conversation

@liugddx

Copy link
Copy Markdown
Member

Summary

  • release telemetry, model-call, and pricing SQLite resources without re-entering root authority during close
  • keep read/write operations lease-bound while making close idempotent after revocation
  • close usage stores in root-identity and marker failure tests, and cover reader cleanup after revocation

Part of #2142.

Validation

  • npm --workspace @maka/core run build
  • npm --workspace @maka/storage run build
  • node --test packages/storage/dist/__tests__/usage-stores.test.js (8/8 pass)
  • npx biome check packages/storage/src/usage-stores.ts packages/storage/src/__tests__/usage-stores.test.ts
  • full storage suite: 734 pass, 12 skip, 1 pre-existing environment-sensitive failure in package-import.test because the current Node process emits the experimental node:sqlite warning; the failure reproduces in isolation and is unrelated to these files

@liugddx

Copy link
Copy Markdown
MemberAuthor

@Astro-Han

@Astro-HanAstro-Han 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.

Thanks — the fix is right and matches every sibling store (usage-stores.ts was the only store wrapping close in the lease; the probe-verified idempotency and the new tests all hold, 8/8 green).

One P2 on the concurrency form of the very scenario this PR fixes: an admitted in-flight write at revocation time still makes close reject — the in-flight op fails with invalid_lease, gets collected into failures, and close's throwDeduplicatedFailures (usage-stores.ts:381) upgrades that benign revocation into a shutdown failure (invalid_lease is itself classified as lifecycle/needsDrain:false at :165-167). Reproduced with a probe: close → StorageRootAuthorityError:invalid_lease, which in the host-kernel path becomes a 'did not cleanly close every resource' exit. Pre-existing, but it is the concurrent form of what this PR claims to fix, and it's a small change (fold lifecycle codes into the expected-failure predicate, or filter them before the aggregation). Happy to defer if you'd rather land this first. P3: writerByLease isn't cleaned on close (siblings delete it), and the new test could assert idempotent double-close.

Approving.

@liugddx

Copy link
Copy Markdown
MemberAuthor

Added aa1fe4ac4 after reproducing the remaining Windows usage-store failure locally.

The failure was not another leaked SQLite lease: Windows rejects renaming a directory that contains the open runtime.sqlite before the assertion begins, and the fixture had placed that setup outside its try/finally. The follow-up now:

  • keeps store/owner shutdown inside the failure-safe lifecycle;
  • retains the rename/replacement contract on platforms that permit it;
  • narrowly skips it on Windows with the platform limitation stated explicitly.

Windows target result: 7 pass / 0 fail / 1 skip for usage-stores.test.js. The new checks are now running.

@liugddx
liugddxforce-pushed the fix/windows-usage-stores-cleanup branch from aa1fe4a to bcb62c7CompareAugust 7, 2026 05:50
@liugddx

Copy link
Copy Markdown
MemberAuthor

The failed required run was unrelated to this PR: the only storage failure was a process crash leaves owned staging that can be reclaimed without touching decoys on Linux (false !== true). Main already fixed that timing assumption in #2385.

Rebased this branch onto current main at 1a7286129, which includes #2385, and retriggered CI. The two PR commits remain isolated on top as 7ca7d4af9 and bcb62c779.

@Astro-Han
Astro-Han merged commit 918f938 into apache:mainAug 7, 2026
11 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@liugddx@Astro-Han