fix(db): wrap ctx.Err() + soften doc comment (bookshelf-adzpx) #1436

Merged

zombor merged 3 commits from bd-bookshelf-adzpx into main 2026-08-13 00:36:13 +00:00

Conversation 3 Commits 3 Files changed 2 +12 -5

zombor commented 2026-08-12 19:57:11 +00:00

Owner

Copy link

Summary

Two non-blocking review minors from code review in internal/db/db.go MigrationStatus:

1. Line 185: Wrap ctx.Err() return with fmt.Errorf and %w to match the wrapping style of other error returns in the function. This enables proper error chain inspection via errors.Is().

2. Lines 161-165: Soften the MigrationStatus doc comment to accurately reflect that the context check is only a pre-call guard—golang-migrate's m.Version() does not accept a context, so cancellation doesn't interrupt an in-flight query.

The existing test for context cancellation (db_test.go:225-233) continues to pass because errors.Is(err, context.Canceled) correctly unwraps %w-wrapped errors.

Test plan

• make test: all unit tests pass
• go vet ./internal/db/...: passes
• Existing "respects context cancellation" test at db_test.go:225-233 still passes with wrapped error

Closes bead bookshelf-adzpx on merge.

## Summary Two non-blocking review minors from code review in internal/db/db.go MigrationStatus: 1. **Line 185:** Wrap ctx.Err() return with fmt.Errorf and %w to match the wrapping style of other error returns in the function. This enables proper error chain inspection via errors.Is(). 2. **Lines 161-165:** Soften the MigrationStatus doc comment to accurately reflect that the context check is only a pre-call guard—golang-migrate's m.Version() does not accept a context, so cancellation doesn't interrupt an in-flight query. The existing test for context cancellation (db_test.go:225-233) continues to pass because errors.Is(err, context.Canceled) correctly unwraps %w-wrapped errors. ## Test plan - make test: all unit tests pass - go vet ./internal/db/...: passes - Existing "respects context cancellation" test at db_test.go:225-233 still passes with wrapped error Closes bead bookshelf-adzpx on merge.

zombor added 1 commit 2026-08-12 19:57:13 +00:00

fix(db): wrap ctx.Err() with %w and soften MigrationStatus doc comment ... 5daab33bf1

Two non-blocking review minors from code review:

1. Line 185: wrap ctx.Err() return with fmt.Errorf and %w to match
   the style of other error returns in MigrationStatus (e.g., lines
   169, 174, 179, 194). This enables proper error chain inspection
   via errors.Is() on the context error.

2. Lines 161-165: soften the doc comment to accurately reflect that
   the context check is a pre-call guard only; golang-migrate's
   m.Version() does not accept a context, so the cancellation check
   does not interrupt an in-flight query.

The existing test at db_test.go:225-233 ("respects context cancellation")
still passes because errors.Is(err, context.Canceled) correctly unwraps
the %w-wrapped error to find the sentinel.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor referenced this pull request from a commit 2026-08-12 20:25:43 +00:00

fix(wfengine): guard EnrichWorkflow activity-failure test against tester idle-panic

zombor added 1 commit 2026-08-12 20:25:44 +00:00

fix(wfengine): guard EnrichWorkflow activity-failure test against tester idle-panic ... 12d205c57e

The Coverage job on PR #1436 failed not on the coverage percentage but on a
genuine test PANIC: internal/wfengine "EnrichWorkflow when activity fails
returns an error" hit the go-workflows tester's hardcoded 10s TestTimeout
idle-panic (tester.go:470) under CI-runner CPU contention. This is the same
root cause already fixed for BulkMetadataWorkflow in bookshelf-19lt (#397):
under heavy parallel package load (NPARALLEL=8, GOMAXPROCS-constrained), the
activity-failure callback can be delayed >10s before delivery to the
workflow event loop.

Apply the same precedented fix: WithTestTimeout(60*time.Second) on this
tester instance. Confirmed locally this test passes standalone; the flake
only manifests under CI's parallel-package scheduling contention.

No production code changed by this commit — internal/db/db.go's %w-wrap +
softened doc comment (the bead's actual scope) are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor commented 2026-08-12 20:35:34 +00:00

Author

Owner

Copy link

Security Review — PR #1436 (bookshelf-adzpx)

Scope: internal/db/db.go (2-line change to MigrationStatus doc comment + error wrapping) and internal/wfengine/simple_workflows_test.go (test-only timeout bump). Very low security surface, reviewed adversarially per the focus areas requested.

Error wrapping (db.go:184) — fmt.Errorf("migration status check cancelled: %w", ctx.Err()) wraps ctx.Err() (always context.Canceled or context.DeadlineExceeded, both static stdlib sentinels) with a static, non-parameterized prefix string. No request data, credentials, DSN, or migration internals are interpolated. errors.Is(err, context.Canceled) callers remain unaffected since %w preserves the chain. No information disclosure.

Doc comment change (db.go:162-165) — comment-only edit describing cancellation-check semantics more precisely (checks before querying, does not interrupt an in-flight query). No code/behavior change from the previous comment; accurate description of the existing select { case <-ctx.Done(): ... } pre-check that was already in place.

Exposure surface — MigrationStatus is called only from cmd/pergamum/migrate.go (the pergamum migrate status CLI subcommand), not from any HTTP handler. Confirmed no unauthenticated or authenticated web route reaches this function; this PR does not add any new call site. No authz surface introduced or altered.

Test file (simple_workflows_test.go) — adds tester.WithTestTimeout(60*time.Second) to NewWorkflowTester construction to fix a CI flake (idle-timeout race under CPU starvation). Test-only, no production code path, no secrets, no assertions weakened — the workflow behavior under test is unchanged, only the tester's idle-timeout budget.

Secrets — none found; no logging of sensitive values in either file.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1436 (bookshelf-adzpx) Scope: `internal/db/db.go` (2-line change to `MigrationStatus` doc comment + error wrapping) and `internal/wfengine/simple_workflows_test.go` (test-only timeout bump). Very low security surface, reviewed adversarially per the focus areas requested. **Error wrapping (`db.go:184`)** — `fmt.Errorf("migration status check cancelled: %w", ctx.Err())` wraps `ctx.Err()` (always `context.Canceled` or `context.DeadlineExceeded`, both static stdlib sentinels) with a static, non-parameterized prefix string. No request data, credentials, DSN, or migration internals are interpolated. `errors.Is(err, context.Canceled)` callers remain unaffected since `%w` preserves the chain. No information disclosure. **Doc comment change (`db.go:162-165`)** — comment-only edit describing cancellation-check semantics more precisely (checks before querying, does not interrupt an in-flight query). No code/behavior change from the previous comment; accurate description of the existing `select { case <-ctx.Done(): ... }` pre-check that was already in place. **Exposure surface** — `MigrationStatus` is called only from `cmd/pergamum/migrate.go` (the `pergamum migrate status` CLI subcommand), not from any HTTP handler. Confirmed no unauthenticated or authenticated web route reaches this function; this PR does not add any new call site. No authz surface introduced or altered. **Test file (`simple_workflows_test.go`)** — adds `tester.WithTestTimeout(60*time.Second)` to `NewWorkflowTester` construction to fix a CI flake (idle-timeout race under CPU starvation). Test-only, no production code path, no secrets, no assertions weakened — the workflow behavior under test is unchanged, only the tester's idle-timeout budget. **Secrets** — none found; no logging of sensitive values in either file. No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-12 20:37:00 +00:00

Author

Owner

Copy link

Adversarial review of PR #1436 (bookshelf-adzpx).

Scope confirmed: (1) %w-wrap of ctx.Err() in MigrationStatus, (2) softened doc comment, (3) bundled flake fix adding tester.WithTestTimeout(60*time.Second) to the EnrichWorkflow tester in internal/wfengine/simple_workflows_test.go. The bundled flake fix is treated as intentional per the dispatch brief (same precedented pattern as bookshelf-19lt/#397).

1. %w wrap — internal/db/db.go:183

return 0, false, fmt.Errorf("migration status check cancelled: %w", ctx.Err())

Correct: errors.Is(err, context.Canceled) still resolves through the wrap. Verified against the existing (unchanged) integration test internal/db/db_test.go:225-233 ("respects context cancellation"), which asserts exactly errors.Is(err, context.Canceled) — passes with the new wrap.

2. Doc comment — internal/db/db.go:161-164

// It checks for context cancellation before querying; does not interrupt an
// in-flight query.

Accurate: m.Version() (golang-migrate) takes no context.Context param, so the select{case <-ctx.Done(): ...} at line 182 is a pre-call guard only, exactly as the new comment states. Old comment ("accepts a context to honor cancellation signals") overstated it as if in-flight cancellation were honored.

3. WithTestTimeout flake fix — internal/wfengine/simple_workflows_test.go
Matches the established bookshelf-19lt idiom exactly (tester.WithTestTimeout(60*time.Second), same call site pattern as bulk_workflow_test.go:809, recalc_scores_workflow_test.go x8). Only the tester construction changed — the "when activity fails" It block (Expect(workflowErr).To(HaveOccurred())) and the permanent-error stub (gowf.NewPermanentError) are untouched, so the assertion still exercises real activity-failure→workflow-error propagation, not a weakened/no-op check. No masking of a real workflow bug: the change only raises the tester's idle-panic threshold, it doesn't alter retry policy, activity registration, or the assertion.

4. New/affected test coverage for db.go
No new test was added in this diff for the wrap — internal/db/db_test.go is untouched (git diff confirms no changes to that file). The pre-existing integration test at db_test.go:225-233 already asserts errors.Is(err, context.Canceled), black-box (package db_test), and continues to pass with the wrapped error. internal/db is excluded from the unit-coverage gate (integration-tested only, per bead comment), consistent with project convention — no coverage regression introduced.

[MINOR] internal/db/db.go:183 — double-wrapped, slightly redundant error message
The new wrap text "migration status check cancelled: %w" combined with the caller's existing wrap in cmd/pergamum/migrate.go:137 ("migration status: %w") produces a slightly redundant final message: migration status: migration status check cancelled: context canceled. Also a minor style deviation from the sibling wraps in the same function, which use short noun-phrases ("migrate source", "migrate driver", "migrate init", "migrate version") rather than a verb-phrase ("... cancelled"). Not worth blocking — purely cosmetic; could tighten to fmt.Errorf("migrate status: %w", ctx.Err()) for consistency with siblings and to avoid the doubled "migration status" wording, but functionally harmless.

No blockers or majors found. The bundled flake fix is verified against the precedented 19lt/#397 pattern and does not weaken the test's real assertions.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

Adversarial review of PR #1436 (bookshelf-adzpx). Scope confirmed: (1) `%w`-wrap of `ctx.Err()` in `MigrationStatus`, (2) softened doc comment, (3) bundled flake fix adding `tester.WithTestTimeout(60*time.Second)` to the `EnrichWorkflow` tester in `internal/wfengine/simple_workflows_test.go`. The bundled flake fix is treated as intentional per the dispatch brief (same precedented pattern as bookshelf-19lt/#397). **1. `%w` wrap — internal/db/db.go:183** ```go return 0, false, fmt.Errorf("migration status check cancelled: %w", ctx.Err()) ``` Correct: `errors.Is(err, context.Canceled)` still resolves through the wrap. Verified against the existing (unchanged) integration test `internal/db/db_test.go:225-233` ("respects context cancellation"), which asserts exactly `errors.Is(err, context.Canceled)` — passes with the new wrap. **2. Doc comment — internal/db/db.go:161-164** ```go // It checks for context cancellation before querying; does not interrupt an // in-flight query. ``` Accurate: `m.Version()` (golang-migrate) takes no `context.Context` param, so the `select{case <-ctx.Done(): ...}` at line 182 is a pre-call guard only, exactly as the new comment states. Old comment ("accepts a context to honor cancellation signals") overstated it as if in-flight cancellation were honored. **3. WithTestTimeout flake fix — internal/wfengine/simple_workflows_test.go** Matches the established bookshelf-19lt idiom exactly (`tester.WithTestTimeout(60*time.Second)`, same call site pattern as `bulk_workflow_test.go:809`, `recalc_scores_workflow_test.go` x8). Only the tester construction changed — the "when activity fails" `It` block (`Expect(workflowErr).To(HaveOccurred())`) and the permanent-error stub (`gowf.NewPermanentError`) are untouched, so the assertion still exercises real activity-failure→workflow-error propagation, not a weakened/no-op check. No masking of a real workflow bug: the change only raises the tester's idle-panic threshold, it doesn't alter retry policy, activity registration, or the assertion. **4. New/affected test coverage for db.go** No new test was added in this diff for the wrap — `internal/db/db_test.go` is untouched (git diff confirms no changes to that file). The pre-existing integration test at db_test.go:225-233 already asserts `errors.Is(err, context.Canceled)`, black-box (`package db_test`), and continues to pass with the wrapped error. `internal/db` is excluded from the unit-coverage gate (integration-tested only, per bead comment), consistent with project convention — no coverage regression introduced. **[MINOR] internal/db/db.go:183 — double-wrapped, slightly redundant error message** The new wrap text `"migration status check cancelled: %w"` combined with the caller's existing wrap in `cmd/pergamum/migrate.go:137` (`"migration status: %w"`) produces a slightly redundant final message: `migration status: migration status check cancelled: context canceled`. Also a minor style deviation from the sibling wraps in the same function, which use short noun-phrases (`"migrate source"`, `"migrate driver"`, `"migrate init"`, `"migrate version"`) rather than a verb-phrase ("... cancelled"). Not worth blocking — purely cosmetic; could tighten to `fmt.Errorf("migrate status: %w", ctx.Err())` for consistency with siblings and to avoid the doubled "migration status" wording, but functionally harmless. No blockers or majors found. The bundled flake fix is verified against the precedented 19lt/#397 pattern and does not weaken the test's real assertions. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-12 20:37:30 +00:00

Author

Owner

Copy link

Adversarial review of PR #1435 (bookshelf-v3bi0).

[MAJOR] static/js/controllers/bookdrop_file_editor_controller.js:489-495 — network-error retry path drops/leaks the queued dispatchCopyEvent flag, defeating the PR's own guarantee in a race
The .catch() branch (network error) does not mirror the .then() branch's flag handling. Compare:

.then() (lines 480-484, correct):

if (ctrl._savePendingAfterInflight) {
  var nextDispatchCopy = ctrl._pendingDispatchCopy;
  ctrl._savePendingAfterInflight = false;
  ctrl._pendingDispatchCopy = false;
  ctrl._doSave(nextDispatchCopy);
}

.catch() (lines 489-495, buggy):

.catch(function () {
  ctrl._saveInFlight = false;
  ctrl._onSaveError("Network error. Please try again.");
  if (ctrl._savePendingAfterInflight) {
    ctrl._savePendingAfterInflight = false;
    ctrl._doSave();   // <-- no argument, and _pendingDispatchCopy is never reset
  } else {
    ctrl._drainFlushResolvers(false);
  }
});

Two concrete failure modes from this one line:

1. Lost dispatch (silent regression of the review workflow). Scenario: _doSave(true) (a copy-triggered save) is in flight; while it's pending, another _doSave() gets queued behind it and sets _pendingDispatchCopy = true. If the in-flight request network-errors, the .catch() branch replays the queued save via ctrl._doSave() — passing undefined instead of true. The replayed save can still succeed and persist the copied fields, but _onSaveSuccess(payload, undefined) will never dispatch bookdrop:metadatacopied, so the row is never auto-checked even though the copy was persisted server-side.

2. Stale flag causes a spurious future dispatch (reintroduces the exact bug this PR fixes). Because _pendingDispatchCopy is left true (never reset to false in the catch branch, unlike the then branch), the next unrelated _doSave() call that happens to race with an in-flight save will OR against that stale true (line 433: ctrl._pendingDispatchCopy = ctrl._pendingDispatchCopy || !!dispatchCopyEvent) and inherit it. A later plain field-edit autosave (not a copy action at all) can then end up dispatching bookdrop:metadatacopied on success — silently auto-checking a row for a save that was never a copy. This is precisely the "row reads as selected but wasn't a real copy" failure class bookshelf-v3bi0 was filed to eliminate, reintroduced via the untested retry path.

Neither path is covered by the new tests — all four new specs in bookdrop_file_editor_controller.test.js use a fetch mock that resolves on the very first call with no queued follow-up save, so the in-flight-queueing branch (lines 432-433, 489-495) is never exercised by any added test.

Fix: mirror the .then() branch's handling in .catch():

.catch(function () {
  ctrl._saveInFlight = false;
  ctrl._onSaveError("Network error. Please try again.");
  if (ctrl._savePendingAfterInflight) {
    var nextDispatchCopy = ctrl._pendingDispatchCopy;
    ctrl._savePendingAfterInflight = false;
    ctrl._pendingDispatchCopy = false;
    ctrl._doSave(nextDispatchCopy);
  } else {
    ctrl._drainFlushResolvers(false);
  }
});

Add a Vitest spec that queues a copy-triggered _doSave(true) behind an in-flight save whose fetch rejects, then resolves the replayed save ok:true, and asserts the event fires exactly once (proves fix #1) — plus a spec proving a later unrelated plain save does NOT dispatch after that network-error race (proves fix #2, i.e. _pendingDispatchCopy is reset).

[MINOR] static/js/test/bookdrop_file_editor_controller.test.js:848-892 — new specs only prove "eventually dispatches/doesn't," not "never fires synchronously before the fetch resolves"
The four new specs (and the updated METADATA COPIED EVENT specs) all await Promise.resolve() three times before asserting, so they can't distinguish "dispatch moved into the resolved-fetch .then()" from a hypothetical implementation that dispatches synchronously at click time but the assertion happens to run after. The failure-path specs (fetch resolves ok:false / rejects) are still a legitimate regression test for the original bug (an unconditional/optimistic dispatch would have fired regardless of outcome, failing those two not.toHaveBeenCalled() assertions), so this is not vacuous — but a stronger spec would additionally assert handler has NOT been called synchronously right after the click, before the first await, to positively pin "no dispatch before the request resolves" rather than inferring it only from the failure-path specs. Non-blocking; existing tests are far from tautological/self-validating.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

Adversarial review of PR #1435 (bookshelf-v3bi0). [MAJOR] static/js/controllers/bookdrop_file_editor_controller.js:489-495 — network-error retry path drops/leaks the queued dispatchCopyEvent flag, defeating the PR's own guarantee in a race The `.catch()` branch (network error) does not mirror the `.then()` branch's flag handling. Compare: `.then()` (lines 480-484, correct): ``` if (ctrl._savePendingAfterInflight) { var nextDispatchCopy = ctrl._pendingDispatchCopy; ctrl._savePendingAfterInflight = false; ctrl._pendingDispatchCopy = false; ctrl._doSave(nextDispatchCopy); } ``` `.catch()` (lines 489-495, buggy): ``` .catch(function () { ctrl._saveInFlight = false; ctrl._onSaveError("Network error. Please try again."); if (ctrl._savePendingAfterInflight) { ctrl._savePendingAfterInflight = false; ctrl._doSave(); // <-- no argument, and _pendingDispatchCopy is never reset } else { ctrl._drainFlushResolvers(false); } }); ``` Two concrete failure modes from this one line: 1. **Lost dispatch (silent regression of the review workflow).** Scenario: `_doSave(true)` (a copy-triggered save) is in flight; while it's pending, another `_doSave()` gets queued behind it and sets `_pendingDispatchCopy = true`. If the in-flight request network-errors, the `.catch()` branch replays the queued save via `ctrl._doSave()` — passing `undefined` instead of `true`. The replayed save can still succeed and persist the copied fields, but `_onSaveSuccess(payload, undefined)` will never dispatch `bookdrop:metadatacopied`, so the row is never auto-checked even though the copy *was* persisted server-side. 2. **Stale flag causes a spurious future dispatch (reintroduces the exact bug this PR fixes).** Because `_pendingDispatchCopy` is left `true` (never reset to `false` in the catch branch, unlike the then branch), the *next* unrelated `_doSave()` call that happens to race with an in-flight save will OR against that stale `true` (line 433: `ctrl._pendingDispatchCopy = ctrl._pendingDispatchCopy || !!dispatchCopyEvent`) and inherit it. A later plain field-edit autosave (not a copy action at all) can then end up dispatching `bookdrop:metadatacopied` on success — silently auto-checking a row for a save that was never a copy. This is precisely the "row reads as selected but wasn't a real copy" failure class bookshelf-v3bi0 was filed to eliminate, reintroduced via the untested retry path. Neither path is covered by the new tests — all four new specs in `bookdrop_file_editor_controller.test.js` use a fetch mock that resolves on the very first call with no queued follow-up save, so the in-flight-queueing branch (lines 432-433, 489-495) is never exercised by any added test. **Fix:** mirror the `.then()` branch's handling in `.catch()`: ```js .catch(function () { ctrl._saveInFlight = false; ctrl._onSaveError("Network error. Please try again."); if (ctrl._savePendingAfterInflight) { var nextDispatchCopy = ctrl._pendingDispatchCopy; ctrl._savePendingAfterInflight = false; ctrl._pendingDispatchCopy = false; ctrl._doSave(nextDispatchCopy); } else { ctrl._drainFlushResolvers(false); } }); ``` Add a Vitest spec that queues a copy-triggered `_doSave(true)` behind an in-flight save whose fetch rejects, then resolves the replayed save `ok:true`, and asserts the event fires exactly once (proves fix #1) — plus a spec proving a later unrelated plain save does NOT dispatch after that network-error race (proves fix #2, i.e. `_pendingDispatchCopy` is reset). [MINOR] static/js/test/bookdrop_file_editor_controller.test.js:848-892 — new specs only prove "eventually dispatches/doesn't," not "never fires synchronously before the fetch resolves" The four new specs (and the updated METADATA COPIED EVENT specs) all await Promise.resolve() three times before asserting, so they can't distinguish "dispatch moved into the resolved-fetch .then()" from a hypothetical implementation that dispatches synchronously at click time but the assertion happens to run after. The failure-path specs (fetch resolves ok:false / rejects) are still a legitimate regression test for the original bug (an unconditional/optimistic dispatch would have fired regardless of outcome, failing those two not.toHaveBeenCalled() assertions), so this is not vacuous — but a stronger spec would additionally assert handler has NOT been called synchronously right after the click, before the first await, to positively pin "no dispatch before the request resolves" rather than inferring it only from the failure-path specs. Non-blocking; existing tests are far from tautological/self-validating. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor added 1 commit 2026-08-13 00:25:35 +00:00

Merge branch 'main' into bd-bookshelf-adzpx ae48859334

zombor merged commit 8f187bd1e2 into main 2026-08-13 00:36:13 +00:00

zombor referenced this pull request from a commit 2026-08-13 00:36:16 +00:00

Merge pull request 'fix(db): wrap ctx.Err() + soften doc comment (bookshelf-adzpx)' (#1436) from bd-bookshelf-adzpx into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1436

No description provided.