jercik commented

Owner

Every read-only DatabaseSync in move-codex-session had node:sqlite's default busy timeout of 0. A read that coincided with another connection rebuilding the WAL index failed at once with database is locked (SQLITE_BUSY_RECOVERY, errcode 261) instead of letting SQLite retry. Another connection rebuilds the index when it is the first to open a WAL database, or checkpoints when it is the last to close one.

Two consequences:

A move fails in preflight when the invoking Codex, which the script deliberately tolerates, opens a database at the wrong moment.
The same unprotected reads run after the source is deleted. A spurious failure there means exit 1 with no JSON after a completed move, the outcome #145 ruled out.

I found it through "keeps a destination writer's history rows when the history copy fails", which flaked with database is locked instead of UNIQUE constraint failed. All 12 captured stack traces failed on the first read of the read-only destination state connection.

An openReadOnly(path) helper now sets a 10 s busy timeout, matching the writers' 10 s and 30 s, and all 17 read-only opens use it. I rejected rewriting the test's poll to keep one connection open, because that would hide the bug.

Under stress (16 CPU hogs, 24 parallel), 4 of 320 runs failed before and 0 of 320 after. In the diagnosis, the failure rate dropped from 1.25% to 0% on macOS and from 3.1% to 0% on Linux (Debian VM). A deterministic reproduction, where a parent holds the index-rebuild locks, failed 3 of 3 before and passed 3 of 3 after.

The new test "waits for the invoker's brief lock on a destination database" fails 3 of 3 without the fix. It holds the lock for 1 s, so it would also pass if the script took more than 1 s between opening the file and first reading it. The measured gap is about 10 ms.

This is not the cause of the older Linux-only "tolerates the invoker…" failure, which did not reproduce in 480 stressed runs.

🤖 Generated with Claude Code

Every read-only `DatabaseSync` in `move-codex-session` had `node:sqlite`'s default busy timeout of 0. A read that coincided with another connection rebuilding the WAL index failed at once with `database is locked` (`SQLITE_BUSY_RECOVERY`, errcode 261) instead of letting SQLite retry. Another connection rebuilds the index when it is the first to open a WAL database, or checkpoints when it is the last to close one. Two consequences: - A move fails in preflight when the invoking Codex, which the script deliberately tolerates, opens a database at the wrong moment. - The same unprotected reads run after the source is deleted. A spurious failure there means exit 1 with no JSON after a completed move, the outcome #145 ruled out. I found it through "keeps a destination writer's history rows when the history copy fails", which flaked with `database is locked` instead of `UNIQUE constraint failed`. All 12 captured stack traces failed on the first read of the read-only destination state connection. An `openReadOnly(path)` helper now sets a 10 s busy timeout, matching the writers' 10 s and 30 s, and all 17 read-only opens use it. I rejected rewriting the test's poll to keep one connection open, because that would hide the bug. Under stress (16 CPU hogs, 24 parallel), 4 of 320 runs failed before and 0 of 320 after. In the diagnosis, the failure rate dropped from 1.25% to 0% on macOS and from 3.1% to 0% on Linux (Debian VM). A deterministic reproduction, where a parent holds the index-rebuild locks, failed 3 of 3 before and passed 3 of 3 after. The new test "waits for the invoker's brief lock on a destination database" fails 3 of 3 without the fix. It holds the lock for 1 s, so it would also pass if the script took more than 1 s between opening the file and first reading it. The measured gap is about 10 ms. This is not the cause of the older Linux-only "tolerates the invoker…" failure, which did not reproduce in 480 stressed runs. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-10 08:25:52 +00:00

fix: move-codex-session should wait out transient locks on its read-only connections

commit-msg / commitlint (pull_request) Successful in 24s

Details

Node tests / node:test (pull_request) Successful in 4m15s

Details

Review / Review (pull_request_target) Successful in 3s

Details

5cb7dbe214

`node:sqlite` opens connections with a busy timeout of 0, so a read-only
connection that coincides with another connection rebuilding the WAL index
(the first connection to open a WAL database does, and the last one to close
it checkpoints) failed at once with `database is locked`. The script's
preflight reads hit this while the invoking Codex opened a database, and the
reads after the source is deleted could turn a completed move into exit 1
with no JSON.

Every read-only open now goes through `openReadOnly`, which waits up to 10 s.
A new test holds a destination database's file lock for 1 s while the script
starts.

This also fixes the flaky "keeps a destination writer's history rows when the
history copy fails" test: 4 of 320 runs failed before under load, 0 of 320
after.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

forgejo-actions commented

2026-10-10 08:27:25 +00:00

Review 01M4JF6P98SPMK1YBAA6MX36PW — head a60b58af9a5828eecaa681d27bd4c4aeb1a2868d

Review — j4k-oss/agent-skills @ `cc41f460d4`

Scope: diff against base tree 5b1aa6fb3b4c
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v4",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (0)

No findings survived.

Reviewed:

general-bug (whole/default): no-claims
writing-quality (whole/default): no-claims
test-trimming (whole/default): claims-emitted
restated-sets (whole/default): no-claims
project-docs (whole/default): no-claims

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (2)
- 01M4JF9YDB9G0XPDMAGG9GRY3J low — The lock test can pass without exercising the read timeout
- 01M4JFA7GBQF8B21RD6Z4V4RXF low — The lock test can pass without exercising the read timeout

Coverage

Coverage pass: 01M4JF728FKJW6AYS34417SF32
Accounting: complete
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	no-claims	1	no
writing-quality	whole	default	no-claims	1	no
test-trimming	whole	default	claims-emitted	1	no
restated-sets	whole	default	no-claims	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M4JF6P98SPMK1YBAA6MX36PW` — head `a60b58af9a5828eecaa681d27bd4c4aeb1a2868d` # Review — j4k-oss/agent-skills @ cc41f460d4d2 Scope: diff against base tree `5b1aa6fb3b4c` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (0) No findings survived. Reviewed: - general-bug (whole/default): no-claims - writing-quality (whole/default): no-claims - test-trimming (whole/default): claims-emitted - restated-sets (whole/default): no-claims - project-docs (whole/default): no-claims ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (2) - `01M4JF9YDB9G0XPDMAGG9GRY3J` low — The lock test can pass without exercising the read timeout - `01M4JFA7GBQF8B21RD6Z4V4RXF` low — The lock test can pass without exercising the read timeout ## Coverage Coverage pass: 01M4JF728FKJW6AYS34417SF32 Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | no-claims | 1 | no | | test-trimming | whole | default | claims-emitted | 1 | no | | restated-sets | whole | default | no-claims | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

jercik force-pushed fix/move-codex-session-read-only-busy-timeout from 5cb7dbe214

commit-msg / commitlint (pull_request) Successful in 24s

Details

Node tests / node:test (pull_request) Successful in 4m15s

Details

Review / Review (pull_request_target) Successful in 3s

Details

to a60b58af9a

commit-msg / commitlint (pull_request) Successful in 22s

Details

Review / Review (pull_request_target) Successful in 3m48s

Details

Node tests / node:test (pull_request) Successful in 4m16s

Details

2026-10-10 08:34:46 +00:00

Compare

jercik commented

2026-10-10 08:40:04 +00:00

Author

Owner

Review 01M4JF6P98SPMK1YBAA6MX36PW on a60b58a left two unadjudicated low claims, both titled "The lock test can pass without exercising the read timeout": 01M4JF9YDB9G0XPDMAGG9GRY3J and 01M4JFA7GBQF8B21RD6Z4V4RXF. The service posted only titles, so I checked the one way I can see it happening.

The test holds the destination state database's lock for 1 s after it first sees the script holding the file. It passes without the timeout only if the script takes more than 1 s between opening the database and first reading it. The test's comment and this PR's body already state that limit, and the gap measured about 10 ms.

With the timeout removed in a scratch copy, the test fails 3 of 3 on a60b58a. With it, the test passes 3 of 3. I found no other way for it to pass without the timeout. A longer hold would only move the margin, and nothing outside SQLite shows when its busy handler starts waiting, so I'm leaving the test as it is and opening no follow-up.

Review `01M4JF6P98SPMK1YBAA6MX36PW` on `a60b58a` left two unadjudicated low claims, both titled "The lock test can pass without exercising the read timeout": `01M4JF9YDB9G0XPDMAGG9GRY3J` and `01M4JFA7GBQF8B21RD6Z4V4RXF`. The service posted only titles, so I checked the one way I can see it happening. The test holds the destination state database's lock for 1 s after it first sees the script holding the file. It passes without the timeout only if the script takes more than 1 s between opening the database and first reading it. The test's comment and this PR's body already state that limit, and the gap measured about 10 ms. With the timeout removed in a scratch copy, the test fails 3 of 3 on `a60b58a`. With it, the test passes 3 of 3. I found no other way for it to pass without the timeout. A longer hold would only move the margin, and nothing outside SQLite shows when its busy handler starts waiting, so I'm leaving the test as it is and opening no follow-up.

🚀 1

jercik merged commit 6d19684cfb into main

2026-10-10 08:40:46 +00:00

jercik deleted branch fix/move-codex-session-read-only-busy-timeout

2026-10-10 08:40:46 +00:00

jercik referenced this pull request from a commit

2026-10-10 08:40:46 +00:00

fix: `move-codex-session` should wait out transient locks on its read-only connections (#148)

Rows
Columns

fix: move-codex-session should wait out transient locks on its read-only connections #148

Review — j4k-oss/agent-skills @ cc41f460d4

Findings (0)

Other claims

Coverage

fix: `move-codex-session` should wait out transient locks on its read-only connections #148

Review — j4k-oss/agent-skills @ `cc41f460d4`