fix(storage): free a dead breaker's lock at once, keep a stopped waiter alive, skip the sweep on a free lock by ThomasK33 · Pull Request #299 · coder/agent-tty

ThomasK33 · 2026-11-03T20:04:38Z

Why

The verifier of #282 found three non-blocking problems in the artifact manifest lock. The operator asked to ship every valid review finding in a follow-up PR.

A breaker that dies while it holds manifest.json.lock.break blocks every capture on that session for up to 60 s. Each capture fails with a retryable STORAGE_WRITE_ERROR after 10 s.
A writer that is stopped for over 60 s while it waits (for example with SIGSTOP) fails when it resumes. Another writer's sweep removed its temp file, and the writer then reports a non-retryable STORAGE_WRITE_ERROR.
Each lock acquisition reads the whole artifacts/ folder to sweep leftover lock files. With 20000 files that costs about 24 ms per lock cycle on main (the verifier measured 110 ms under load).

What changed

All changes are in src/storage/artifactManifest.ts and its README.

A breaker that died holding the break file no longer blocks writers for 60 s. The break file and the new announcement (item 5) hold the breaker's record, so a writer removes them at once when the breaker is dead. Both stay a 60 s lease, so they also go after 60 s while the breaker lives (for example when it is stopped), as before.
Before each attempt, a waiting writer sets its temp file's times (refreshTemporaryFile). When the temp file is gone, the writer writes it again and keeps waiting.
Only a writer that found the lock taken (it waited or broke a stale lock) runs the sweep. A writer that finds the lock free does not read artifacts/.
The sweep removes a temp file when its owner is gone by the rule of a lock, and a break file or announcement by the lease rule of item 1. A dead owner's files go at once (before: after 60 s). A live waiter's temp file is never swept on Linux.
Breakers act one at a time by construction (the verifier's finding N1 on 4dd5616). Before a breaker removes anything, it announces itself with manifest.json.lock.breakers/<id>, a link of its temp file under a name that no other writer uses, in a folder that holds only announcements. It then lists that folder, so the listing does not grow with the artifacts (the delta verifier's livelock on ce57bda, item 6). When another breaker's announcement is there and not stale, it removes its own announcement and waits. It removes stale announcements, which is safe because their names are unique. The breaker that finds itself alone still takes the fixed manifest.json.lock.break file, which keeps out agent-tty versions without announcements.
A breaker that met another one waits a random 0.5 to 1.5 times a backoff of 5 ms that doubles with each meeting, up to 200 ms. On ce57bda, breakers listed the whole artifacts/ folder while announced and retried after 25 to 75 ms, so with 20000 files 30 writers kept seeing each other until all timed out. The folder makes the window short, and the growing backoff spreads breakers that still meet.
A writer checks its deadline right after a failed link, before a break attempt. Before, it checked after the attempt, so a writer stopped (SIGSTOP) inside the attempt resumed past its deadline and timed out although the owner had let go meanwhile. stopped-waiter.sh hit that in 1 of 5 runs on 9717067. A writer that times out no longer breaks a stale lock on its way out. The next writer does.

Behavior change: the temp file of a waiter that died now stays until a later writer finds the lock taken. Before, the next writer removed it after 60 s. The file is small, hidden from inspect, and gc or destroy removes it with the session. The README says so.

Why two writers cannot be inside the lock

Invariant: a lock file that holds a live writer's token is removed only by that writer.

Only a writer whose token is in manifest.json.lock runs its update. So the invariant means at most one writer is inside. Each step keeps it:

Take: link(tmp, lock) creates the lock only when no lock file exists. A token enters the lock only when the lock is absent, and the lock is never replaced in place.
Release: the owner re-reads the lock and removes it only when it still holds the owner's token. By the invariant, nobody else removes that lock between the re-read and the rm, so the rm removes the owner's own lock.
Break: a breaker removes the lock only after these three things:
- It judged the lock's bytes stale: the owner is dead, or it reads unknown and the lock is more than 60 s old.
- It is the only breaker in its section, the time from its announcement until it removes the announcement (invariant S below).
- It re-read the same bytes inside the section.
Between that re-read and the rm, no other writer can remove the lock: the owner is gone, and S keeps every other breaker out. No writer can create a lock while one exists. So the rm removes exactly the stale lock that the breaker judged, never a live writer's lock.

Invariant S: at most one writer is in the break section at a time.

Each breaker links its announcement into the announcement folder before it lists that folder, and it removes the announcement only after its last removal. Announcements exist only in that folder.
A directory listing returns every file that existed when the listing started and was not removed during it (POSIX readdir).
Take two breakers A and B that were both in the section, and say B's listing started later. B's link finished before B's listing started. A's link finished before A's listing started, so before B's listing started. A was still in the section, so its announcement was still there. B's listing therefore returned A's announcement, and B waited. So both cannot be in the section.
A breaker removes another breaker's announcement only when that breaker is dead or the file is more than 60 s old. The names are unique and a dead writer makes no new one, so this removal cannot hit a live breaker's fresh announcement.
The folder is created with mkdir (no recursive) and removed with rmdir, which removes only an empty folder. So removing the folder never removes an announcement. A breaker lists the folder only while its own announcement is inside, so the folder exists then. When another breaker removes the empty folder between a breaker's mkdir and its link, the link fails, the breaker has no announcement, and it does not act. It tries again later.

The invariant does not hold in three cases, and the README lists all three as limits:

A writer with an unknown owner stops for over 60 s inside its update.
A breaker stops for over 60 s inside its section.
A writer from an agent-tty version without announcements takes part. The fixed break file still keeps such writers apart as well as before, but removing a stale break file reads it and removes it by path in two steps.

Why not break by renaming the lock to a unique name and then checking its bytes: that was this lock's design at 9e20f67 in #282. When the bytes do not match, the rename has already moved a live writer's lock away. Putting it back is a second step, and the verifier of #282 showed a third writer entering between the two steps (finding F1). Node has no atomic exchange (renameat2), so no rule for the mismatch case can undo the move safely.

Verification

Red first: commit 41fc500 adds four tests to test/unit/storage/artifactManifestLock.test.ts. All four fail on main: the dead breaker blocks (test timeout), the swept waiter fails with STORAGE_WRITE_ERROR, the sweep keeps a dead owner's fresh temp file, and a free lock still sweeps. They pass with the fix.
Codex round 1 found that a stopped live breaker's break file never expired. Commit 4dd5616 restores the 60 s lease, with a test that fails on 8c36ac8. vitest run test/unit/storage: 11 files, 135 tests pass. test/integration/artifact-manifest-race.test.ts: 2 tests pass.

Live runs with the verifier's scripts from fix: lock the artifact manifest so concurrent captures keep every entry #282, built CLI, main vs this branch:

Script	main	this branch
`breaker-dies.sh`: export after a breaker was killed holding a fresh break file	exit 8 `STORAGE_WRITE_ERROR` after 10.5 s	exit 0 after 0.55 s
`stopped-waiter.sh`: waiter stopped for 65 s, its temp file swept, then SIGCONT	exit 8 `STORAGE_WRITE_ERROR`, not retryable	exit 0
`bench.mjs`: ms per lock cycle with 0 / 1000 / 20000 files in `artifacts/`	1.41 / 3.50 / 23.89	1.34 / 1.14 / 1.20

On 38f9fc3: breaker-dies.sh exit 0 after 1.1 s, and stopped-waiter.sh exit 0 in 4 of 4 runs.

Finding N1 of the verifier on 4dd5616 (two writers inside after a dead breaker):
- Red first: commit 0e8323c turns the verifier's trace T1 into a deterministic unit test, with one AsyncLocalStorage context per writer and rm hooks. It fails on 4dd5616 (2 writers inside) and passes with the announcements (10 of 10 runs).
- The verifier's interleave3.mjs T1 cannot reach its second pause point on this head. W1 waits while W2 holds its announcement, so the script waits forever for W1 to reach the lock's rm.
- stale-stress-brk.sh with a dead owner's lock and a dead breaker's break file: 0 of 20 rounds with 30 writers and 0 of 40 rounds with 8 writers had two writers inside, with no errors. 4dd5616 gave 1 of 20 rounds in the same 30-writer run.
- N2: the new test "removes a dead breaker's fresh break file once it had to wait for a live lock" fails when the sweep judges break files by age only (mutation M6).
The delta verifier's livelock on ce57bda (30 writers, a dead owner's lock and 20000 files in artifacts/: all writers timed out):
- Red first: commit aabe059 adds a deterministic test. While a breaker's announcement exists, no listing of artifacts/ may run. It fails on ce57bda and passes now. (Commit 6a994b5 first tried 30 writers in one process. That measures 30 waiters that share one event loop and read /proc synchronously on every poll, which no real session has, so aabe059 replaces it.)
- The verifier's multi-process fuzz.mjs, 30 writers × 5 rounds, dead owner, 20000 files, no delays: ce57bda 0 of 150 succeeded (150 timeouts). The final head 38f9fc3: 150 of 150, the slowest writer 5.9 s at load 77. 4dd5616 measured 4.8 s on the verifier's host. Most of that time is the 30 sweeps of 20000 names, one by each writer that waited.
- With random delays at every lock operation, a dead lock, a dead break file, a dead announcement and 20000 files, 30 writers × 5 rounds: 150 of 150 (9717067). On 38f9fc3, with 10% of the writers killed at a random step, 30 writers × 10 rounds: SX (a dead lock and a dead break file) 269 of 269 survivors, and SXD (also a dead announcement, with delays) 269 of 269. ce57bda gave 269 of 269 in the same SX run. No run had two writers inside.
- I also tried a 50 ms backoff base and an 800 ms cap. Under load they made the slowest writer slower, so the head uses 5 ms and 200 ms.
- The verifier's model.py (exhaustive interleavings) found no violation: 3 writers with 1 crash in S, SX, SXD and SD, and 4 writers in SX and SXD. The model describes the announcements as a set that one listing reads, which the folder keeps.
Item 7: commit 0f6abb1 adds a test where the clock jumps past the deadline while the writer judges a lock whose owner then lets go. It fails on 9717067 and passes on 38f9fc3.
The delta verifier of 38f9fc3 found four guards that no test covered. Commit d1c9f2d adds one test per guard, and each fails when its guard is removed (the verifier's mutate2.py): Q2 the retry after the announcement folder was removed between mkdir and link, Q3 the growing backoff, Q7 the sweep of the announcement folder, Q8 a breaker's removal of a dead announcement.
vitest run test/unit/storage: 11 files, 143 tests pass. test/integration/artifact-manifest-race.test.ts: 2 tests pass.
tsc --noEmit, oxlint src test vitest.config.ts, and oxfmt . --check pass. The lifecycle facts (facts.mjs) equal coverage.json, so check.sh was not needed.

Limit (in the README): a writer with an unknown owner that stops for over 60 s between setting its temp file's times and the link still fails when another writer removes the temp file in that window.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: xhigh

…nd sweep cost Four tests fail on main: - a dead breaker's break file blocks every writer for 60 s; - a waiter whose temp file another writer removed fails with a non-retryable STORAGE_WRITE_ERROR; - a sweep keeps a dead owner's fresh temp file; - an acquisition that finds the lock free still reads the artifacts folder (it removes an old leftover).

…skip the sweep on a free lock - A breaker that died holding manifest.json.lock.break blocked every writer on that session for 60 s. The break file holds the breaker's record, so writers now judge it like a lock: a dead breaker's break file goes at once, an unknown one after 60 s. - A waiter stopped for over 60 s found its temp file swept by another writer and failed with a non-retryable STORAGE_WRITE_ERROR when it resumed. It now writes the temp file again before the next attempt, and the sweep keeps temp and break files whose owner is alive. - The sweep read the whole artifacts folder on every acquisition (about 110 ms per lock cycle with 20000 files). Only a writer that found the lock taken runs it now. It also removes the files of dead owners at once instead of after 60 s.

ThomasK33 · 2026-11-03T20:04:46Z

@codex review

chatgpt-codex-connector · 2026-11-03T20:05:03Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-04T01:56:33.023847Z	`f7f0c28`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c36ac896c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-03T20:08:13Z

+    if (breaker !== null && isOwnerGone(breaker, timing)) {
      await removeIfUnchanged(breakPath, breaker.contents);


Retain an age limit for live breakers

When a Linux breaker is SIGSTOPed or otherwise hangs after creating .break, judgeHost continues to report it as alive, so isOwnerGone never removes the break file regardless of age. Every subsequent writer therefore times out indefinitely while the stale manifest lock remains; previously the 60-second age lease allowed another breaker to recover safely, and the updated storage README still promises that a stopped breaker eventually loses the file. Keep the age-based expiry for break files in addition to removing dead breakers immediately.

Useful? React with 👍 / 👎.

Valid. Fixed in 4dd5616: the break file is a lease again (isStaleBreakFile). It goes when its breaker is dead, or when it is more than 60 s old, also while the breaker lives. The sweep uses the same rule for break files. New test: "keeps the break file of a live breaker until it is older than unknownOwnerStaleMs" (it fails on 8c36ac8 with a test timeout).

Codex review on #299: with the owner judgement alone, a breaker that is stopped (SIGSTOP) after it took the break file reads as alive on Linux, so its break file never went and every writer timed out while the stale lock stayed. The break file is a lease again: it also expires after 60 s while its breaker lives. A dead breaker's file still goes at once.

ThomasK33 · 2026-11-03T20:15:13Z

@codex review

chatgpt-codex-connector · 2026-11-03T20:18:22Z

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 4dd5616848

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

… pin the sweep of its fresh break file - The verifier's trace T1 on #299, as a deterministic test: a writer that stops while it removes a dead breaker's break file later removes another writer's new break file by path, and two writers end up inside. It fails on 4dd5616 (2 inside). - A writer that waited for a live lock removes a dead breaker's fresh break file in its sweep. It passes, and it fails when the sweep judges break files by age only.

The verifier of #299 (N1) found that a writer that stops while it removes a dead breaker's break file can later remove another breaker's new break file by path, so two breakers break and two writers end up inside. The fixed break file cannot keep breakers apart, because a stale one is read and removed in two steps. A breaker now first links its temp file to a name that only it uses, manifest.json.lock.breaker-<id>, and lists the artifacts folder. When another breaker's announcement is there and not stale, it removes its own and waits with jitter. Of two breakers, the one that lists later sees the other, so at most one breaker is in the section, and only it removes the lock. Stale announcements go by the break-file lease rule; their names are unique, so removing one by path cannot hit a live breaker's. The acting breaker still takes the fixed break file to keep out versions without announcements. The README lists the limits.

ThomasK33 · 2026-11-03T22:24:48Z

@codex review

chatgpt-codex-connector · 2026-11-03T22:27:54Z

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: ce57bdad27

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…a large artifacts folder The delta verifier of #299 found that 30 writers on a dead owner's lock all time out when artifacts/ holds 20000 files: each breaker lists the whole folder while its announcement exists, so they keep seeing each other. The test fails on ce57bda (30 of 30 rejected).

…he artifacts folder The in-process burst test of 6a994b5 measured the cost of 30 waiters in one event loop (judgeHost reads /proc synchronously on every poll), which no real session has: the host serializes its appends. It is replaced by a deterministic test of the mechanism behind the livelock: while a breaker's announcement exists, no listing of artifacts/ may run. It fails on ce57bda. The multi-process 30-writer, 20000-file burst is a stress check (the verifier's fuzz.mjs), not a unit test.

… off on each meeting The delta verifier of #299 found a livelock after a crash in a large session: each breaker listed the whole artifacts folder while its announcement existed (tens of ms at 20000 files) and retried after 25 to 75 ms, so 30 breakers kept seeing each other and all timed out. Announcements now live in artifacts/manifest.json.lock.breakers/, which holds nothing else, so the listing does not grow with the artifacts. The folder is made with mkdir (no recursive) and removed with rmdir, which removes only an empty folder, so no announcement goes with it. A link into a folder that another breaker just removed fails, and the breaker tries again without an announcement. A breaker that met another waits a random 0.5 to 1.5 times a backoff of 5 ms that doubles per meeting, up to 200 ms.

…e the lock is free A writer stopped while it judges the lock checks its deadline after the attempt and times out on resume, although the owner let go while it was stopped. The verifier's stopped-waiter script hit this once in five runs. The test fails on the current code.

…after the break attempt A writer stopped inside a break attempt resumed past its deadline and threw at once, although the owner had let go in the meantime. The check now comes before the break attempt, so the writer tries the link once more after it resumes. A writer that times out no longer breaks a stale lock on its way out; the next writer does.

ThomasK33 · 2026-11-04T01:00:42Z

@codex review

chatgpt-codex-connector · 2026-11-04T01:02:56Z

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 38f9fc3ddc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The delta verifier of #299 found four guards that survive mutation: - Q2: a link into an announcement folder that another breaker removed after the mkdir fails with ENOENT, and the writer tries again. - Q3: the meeting backoff grows. With a live announcement that stays, a writer lists the folder fewer than 40 times in 1.5 s. - Q7: the sweep removes a dead breaker's announcement and the empty folder after the writer waited for a live lock. - Q8: a breaker removes a dead breaker's announcement and breaks the lock at once. Each test fails when its guard is removed (the verifier's mutate2.py).

ThomasK33 · 2026-11-04T01:38:32Z

@codex review

chatgpt-codex-connector · 2026-11-04T01:40:38Z

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: d1c9f2dd2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…sweep # Conflicts: # src/storage/artifactManifest.ts

ThomasK33 · 2026-11-04T01:54:19Z

@codex review

chatgpt-codex-connector · 2026-11-04T01:56:31Z

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: f7f0c28502

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

ThomasK33 added 2 commits November 3, 2026 19:54

chatgpt-codex-connector Bot reviewed Nov 3, 2026

View reviewed changes

ThomasK33 added 2 commits November 3, 2026 22:11

ThomasK33 added 5 commits November 3, 2026 23:54

Merge remote-tracking branch 'origin/main' into fix/lock-breaker-and-…

f7f0c28

…sweep # Conflicts: # src/storage/artifactManifest.ts

		if (breaker !== null && isOwnerGone(breaker, timing)) {
		await removeIfUnchanged(breakPath, breaker.contents);

Conversation

ThomasK33 commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Why two writers cannot be inside the lock

Verification

Uh oh!

ThomasK33 commented Nov 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

ThomasK33 Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

ThomasK33 commented Nov 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 3, 2026

Uh oh!

ThomasK33 commented Nov 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 3, 2026

Uh oh!

ThomasK33 commented Nov 4, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 4, 2026

Uh oh!

ThomasK33 commented Nov 4, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 4, 2026

Uh oh!

ThomasK33 commented Nov 4, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 4, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ThomasK33 commented Nov 3, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Nov 3, 2026 •

edited

Loading