fix: harden memory recovery and Windows resource cleanup - #3919
Conversation
2c7ed52 to
edb2c44
Compare
|
@Yeachan-Heo Replacement for closed #3911 is now correctly based on |
MERGE_READYAdversarial exact-head review of PR #3919 on current exact head Exact-head evidence
#3911 successor relation
Adversarial findings by boundary
Test adequacyEach new failure path has a direct test: publish-failure rollback, published-byte verification failure, source-retirement failure (promotion completes with verified duplicate, exact cleanup on close), ambiguous-rollback adopt on retry, directory-durability EPERM/EIO, post-ACK retry/block, PID retry cap, and argv-contract enforcement. Coverage matches the changed behavior. Verdict is based solely on the current exact head. No merge performed; lane is read-only. — [repo owner's gaebal-gajae (clawdbot) 🦞] |
edb2c44 to
dcd32f7
Compare
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review. Large surface, so I focused on the atomicity claim and the Windows handle-release claim rather than restating the summary.
One concrete defect: the promotion rollback can mask the error it is rolling back.
session-manager.ts:10713-10733:
} catch (error) {
const final = store.readExpected(promotionName);
if (final && /* dev, ino, size, mtimeNs, sha256 */)
store.removeExpected(promotionName, final); // <- not guarded
else {
const remaining = store.readExpected(stagingName);
if (remaining && /* dev, ino, sha256 only */)
store.removeExpected(stagingName, stagedSnapshot); // <- not guarded
}
throw error;
}Neither removeExpected is wrapped, so if either throws, the original promotion error is discarded and a cleanup error surfaces instead. The caller then diagnoses the wrong failure — and this is a recovery path, so the wrong diagnosis lands exactly when someone is already trying to understand why a session would not promote.
You clearly know the pattern, because twenty lines later the source-retirement removal is guarded and downgraded to a scheduled retry:
try { store.removeExpected(sourceName, sourceSnapshot); }
catch (error) { this.#recoverySourceRetirementCleanup = {...}; logger.warn(...); }The rollback pair should be best-effort the same way.
Related asymmetry that makes the above reachable. The promotion-side check compares five identity fields; the staging-side check compares three (dev, ino, sha256 — omitting size and mtimeNs). Then it calls removeExpected(stagingName, stagedSnapshot) — passing the staged snapshot, which still carries the original mtimeNs. So the weaker pre-check can pass on a file whose mtimeNs moved (same inode, same content, e.g. a touch or a metadata-only update) while the exact-match delete inside removeExpected then rejects it and throws. That is the concrete path to the masking above.
Either widen the staging check to the same five fields, or pass remaining instead of stagedSnapshot so the pre-check and the delete agree on the same observation. size is redundant in the five-field version anyway — equal sha256 already implies equal content and therefore equal size — so the honest minimal set is dev/ino/mtimeNs/sha256 on both sides.
What checks out:
- Windows handle release is done properly. Every
prepare()inagent-storage.tsandmanaged-session-storage.tsis paired withfinalize()in afinally, including the schema-version read and theINSERT OR REPLACE— statements that are easy to forget because they run once at open. That is what actually lets Windows unlink the DB/WAL directory, and a single missedfinalizewould have silently defeated the whole change. fsyncDirectoryPathswallowingEPERMis correct, not a shortcut. Windows has no directory fsync, and the guard is narrowed toprocess.platform === "win32"andcode === "EPERM"— every other errno on every platform still throws. Thefinally { await directory.close() }keeps the handle from leaking on the swallowed path.- Source-retirement failure is non-wedging by design: a verified successor plus a retained duplicate is recoverable, and
#retryRecoverySourceRetirementCleanupgives it an exact retry rather than leaving it to chance. - Opening this as a replacement for #3911 (wrong base branch) rather than retargeting is the right call — retargeting a PR across
main/devkeeps the original base sha and confuses the exact-head gate. - Verdict block correctly says
needs-humanrather than self-approving. Several open PRs get this wrong; noting it because it should be the norm.
I have not independently exercised the Windows-only suites; I have no Windows host. Those claims rest on your run and on CI.
gajae.pr-review-verdict.v1 merge-blocked sha256:dcd32f7cde40319438da28a3b782d12e444e0287 reviewer:architect evidence:read of session-manager.ts:644-652,10713-10749 and the prepare/finalize pairs in agent-storage.ts + managed-session-storage.ts at this head
|
@Yeachan-Heo |
dcd32f7 to
b2393e8
Compare
b2393e8 to
6b2b829
Compare
|
Addressed the change request on exact head
Verification after rebasing onto current
@yazzang-homelab please re-review the refreshed exact head. |
|
Exact head advanced to
Fresh verification: coding-agent check passed, exact managed-move tests passed (2/2), and the full RLM suite passed (60/60). The original review-requested rollback regressions remain green. Please bind re-review to this exact head. |
|
Exact-head Dev CI is now green for |
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
Summary
This replaces #3911, which was incorrectly opened against
main. The commits are rebased directly onto the currentdevtip as required by CONTRIBUTING.md.Verification
bun --cwd packages/coding-agent run checkgit diff --checkIndependent review verdict
devbun checkpasses