Skip to content

fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) - #267

Open
rapiercraft-forgedock[bot] wants to merge 2 commits into
stagingfrom
forgedock/issue-197-6-4ac5-a942-54cf69a16aa5
Open

fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186)#267
rapiercraft-forgedock[bot] wants to merge 2 commits into
stagingfrom
forgedock/issue-197-6-4ac5-a942-54cf69a16aa5

Conversation

@rapiercraft-forgedock

Copy link
Copy Markdown

Closes #197

ForgeDock verified handoff

  • Run: run_efaff56d-5576-4ac5-a942-54cf69a16aa5
  • Build Packet: art_d2ae6f6d-cb1f-4a2f-a407-66bc6f08609c (durable artifact on the linked issue)
  • Build Result: art_302ff8c9-a774-469a-aadb-46ef93d3d815 (durable artifact on the linked issue)
  • Verified head: 11cdeeb102dd39cefb2ce795e17e845a2c39f4f5

Summary

Completed the retained lease bootstrap repair and added a concrete SQLite repository revision: fresh local witnesses seed authenticated epoch 0, SQLite keeps the empty state at epoch 0, and monotonic conditional witness acceptance prevents stale continuity reads from regressing fencing state.

Changed paths

  • src/adapters/sqlite/lease-witness.test.ts
  • src/adapters/sqlite/lease-witness.ts
  • src/adapters/sqlite/orchestration-admission.test.ts
  • src/adapters/sqlite/sqlite-repositories.test.ts
  • src/adapters/sqlite/sqlite-repositories.ts

Acceptance evidence

  • passed — criterion-1: The reviewed failure sequence is closed: a newly bootstrapped checkout has a cryptographically verified epoch-0 checkpoint, a fresh SQLite lease_state remains at epoch 0, and the first acquire succeeds with epoch 1 and verified continuity instead of raising LeaseContinuityError.: bootstrapLocalLeaseWitness writes a signed epoch-0 checkpoint with writeInitialCheckpoint and verifies it before the atomic witness-directory rename. SqliteRepositories explicitly initializes EMPTY_LEASE_EPOCH at 0 and preserves exact witness/local-state equality; sqlite-repositories.test.ts covers the real bootstrap → createConfiguredLeaseWitness → fresh file-backed repository → acquire sequence and asserts first epoch 1 continuity. Controller-observed subject evidence: GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 body: ## Problem Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-2-of-10:correctness-lease-bootstrap-epoch-mismatch **Reviewer sessio…
  • passed — criterion-2: Bootstrap authentication and installation safety remain intact: the epoch-0 checkpoint is signed by the generated private key and matching keyId before installation, existing key/configuration material is never overwritten, and missing, malformed, wrong-key, or tampered checkpoints are never treated as epoch 0.: lease-witness.ts signs and verifies the generated epoch-0 checkpoint before installation, retains wx/no-overwrite checks and private-key protections, and createConfiguredLeaseWitness verifies key pairing and the retained checkpoint. lease-witness.test.ts covers epoch/keyId/signature, repeat-bootstrap non-overwrite, and missing, malformed, wrong-key, and tampered checkpoint rejection. Controller-observed subject evidence: GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 body: ## Problem Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-2-of-10:correctness-lease-bootstrap-epoch-mismatch Reviewer sessions: b54a4e25 Confidence: HIGH Severity:
  • passed — criterion-3: Existing lease history remains fail-closed: exact witness/local-maximum equality and monotonic fencing are preserved, a signed checkpoint mismatch in a store with history is not automatically adopted, and only an explicit higher authenticated re-enrollment can recover the store while stale lease rows remain fenced.: SqliteRepositories retains exact #assertLeaseContinuity checks and explicit reEnroll recovery without automatic checkpoint adoption. #localMaximum rejects missing/invalid durable epoch state, while #acceptWitness uses a conditional monotonic update so a stale read cannot overwrite a newer fencing epoch. sqlite-repositories.test.ts covers signed rollback, higher-but-unenrolled and tampered checkpoints, latching across acquire/heartbeat/release/guard, explicit higher re-enrollment, stale-row fencing, and higher recovery epochs. Controller-observed subject evidence: GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 body: ## Problem Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: `correctness…
  • passed — criterion-4: The existing caller boundary remains usable after bootstrap: the work-on acquisition path and the orchestration admission used by orchestrate/resume can obtain their first lease/claim before dispatch, while lease continuity errors still deny dispatch rather than being repaired or ignored.: The shared createConfiguredLeaseWitness and SqliteRepositories caller boundary in cli/main.ts remains unchanged and is exercised through the adapter tests. orchestration-admission.test.ts covers a first LeaseBackedOrchestrationExecutionAdmission claim from a fresh bootstrap at epoch 1 with release, and verifies an injected LeaseContinuityError prevents worker dispatch. Controller-observed subject evidence: GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 body: ## Problem Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-2-of-10:correctness-lease-bootstrap-epoch-mismatch Reviewer sessions: b54a4e25 Confidence: HIGH Severity: HIGH **Control…
  • passed — criterion-5: Restart and concurrency behavior remains monotonic and exclusive: an active lease cannot be stolen, an expired lease reacquires with a strictly higher epoch and a new token, persisted state survives a repository restart, and heartbeat/guard/release or cancellation failures remain fail-closed.: The existing SQLite transaction/busy-retry and witness compare-and-advance paths remain in place, with the new conditional #acceptWitness monotonicity guard. sqlite-repositories.test.ts and orchestration-admission.test.ts cover active exclusion, concurrent repository admission, restart persistence, expiry/new token/higher epoch, heartbeat/guard/release continuity denial, stale-row fencing, and admission expiry failure. Controller-observed subject evidence: GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint (review finding — PR #186) #197 body: ## Problem Fresh local lease bootstrap leaves the SQLite epoch behind the signed checkpoint Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-2-of-10:correctness-lease-bootstrap-epoch-mismatch Reviewer sessions: `b54a4e25…

Verification

  • passedgit diff --check — warning: in the working copy of 'src/adapters/sqlite/orchestration-admission.test.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/adapters/sqlite/sqlite-repositories.test.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/adapters/sqlite/sqlite-repositories.ts', LF will be replaced by CRLF the next time Git touches it
  • passedC:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run build — > forgedock@1.8.0 build | > tsc -p tsconfig.json
  • passedC:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run docs:build — - generating sitemap... | ✓ generating sitemap... | build complete in 5.30s.
  • passedC:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js test — # skipped 8 | # todo 0 | # duration_ms 82725.5009

Residual risks

  • The granted tool surface did not include a process/typed verification runner, so npm test, npm run build, git diff --check, and the staging controller gate were not executed here; the controller must independently rerun the frozen verification plan.
  • Legacy already-installed epoch-1 local witnesses remain intentionally incompatible with an untouched empty lease database; migration remains out of scope.
  • The retained checkpoint and SQLite commit remain separate durability boundaries; existing compare-before-row-assignment and busy-serialization behavior is preserved.

@rapiercraft-forgedock

Copy link
Copy Markdown
Author

ForgeDock Review Evidence

One bounded projection for the complete frozen reviewer wave. The controller's consolidated Review Verdict remains authoritative.

Completed groups

review-correctness · correctness · completed
  • Session lineage: 01a00947-1e10-7338-8367-413ebe0c26e0
  • Summary: Found one blocking fencing gap: the new epoch-zero bootstrap is accepted by databases that contain legacy epoch-zero lease rows, even though the packet requires existing lease history to remain fail-closed.
  • Findings:
    • HIGH · Epoch-zero bootstrap does not prove the SQLite store is emptysrc/adapters/sqlite/sqlite-repositories.ts — constructor lease schema; acquire; #assertLeaseContinuity
      • Evidence: bootstrapLocalLeaseWitness now installs a signed epoch-0 checkpoint (lease-witness.ts, writeInitialCheckpoint). SqliteRepositories creates/retains lease_state.max_epoch=0, and the leases table uses epoch INTEGER NOT NULL DEFAULT 0 (sqlite-repositories.ts, constructor). #assertLeaseContinuity only requires witness epoch equality with #localMaximum; it never checks whether leases already contains rows. Consequently, a pre-fencing database migrated by this constructor with an epoch-0 active or expired row is accepted as compatible with the freshly bootstrapped witness: heartbeat/release/guard can continue for an epoch-0 row, and acquire can delete an expired row and advance to epoch 1. The newly added file-backed tests create empty databases, so they do not exercise this legacy-history path.
      • Remediation: When the local maximum and verified witness are both epoch 0, require affirmative evidence that the lease store has no existing lease rows (and fail closed if legacy rows are present). Require an explicit higher authenticated reEnroll before recovery; the existing recovery epoch then fences the epoch-0 rows. Add a regression using a pre-populated/legacy epoch-0 lease row and assert acquire, heartbeat, release, and guard deny until re-enrollment.
review-data · data · completed
  • Session lineage: 01a00947-1dd5-7a6c-b86e-e06daab8cffb
  • Summary: Found a global re-enrollment fencing gap: stale lease rows can become usable again after another lease is acquired or after a repository restart.
  • Findings:
    • HIGH · Higher re-enrollment fence is process-local and is cleared before all stale rows are fencedsrc/adapters/sqlite/sqlite-repositories.ts — '#recoveryEpoch'; 'acquire'; 'heartbeat'; 'release'; 'guard'; 'reEnroll'
      • Evidence: reEnroll stores the recovery epoch only in the instance field #recoveryEpoch. heartbeat, release, and guard reject an old lease row only while that field is set. acquire unconditionally sets #recoveryEpoch = undefined after any successful insertion, even when it acquires a different item and leaves older lease rows untouched. For example, leases A(epoch 1) and B(epoch 2) can exist, an authenticated re-enrollment can move the witness/local maximum to 10, and acquiring a new item C at epoch 11 clears the flag; the old token for A then passes continuity and the row.epoch < #recoveryEpoch check is no longer performed. A separately opened repository has the same problem immediately because the recovery epoch is not persisted. Thus an old holder can heartbeat, pass its guard, or release after the explicit recovery that should fence it.
      • Remediation: Persist a recovery-fence epoch (or equivalent authenticated recovery marker) in the durable lease state and consult it for every heartbeat, guard, release, and acquire across repository instances and restarts. Do not clear the marker after the first unrelated successful acquisition; add a regression with two pre-existing lease rows, recovery, acquisition of a third item, repository reopen, and denial of all old tokens.
review-concurrency · concurrency · completed
  • Session lineage: 01a00947-1e4e-7f38-a6d2-f59a0af03ca9
  • Summary: The change closes the fresh epoch mismatch, but two concurrency safety gaps remain around historical lease fencing and recovery state.
  • Findings:
    • HIGH · Explicit re-enrollment does not fence stale leases across repository instancessrc/adapters/sqlite/sqlite-repositories.ts — #recoveryEpoch, reEnroll(), acquire(), heartbeat(), release(), and guard()
      • Evidence: SqliteRepositories.reEnroll() records the authenticated recovery boundary only in the private #recoveryEpoch. heartbeat(), release(), and guard() reject a pre-re-enrollment row only when that private field is set. A second SqliteRepositories using the same database, or a repository reopened after re-enrollment, has #recoveryEpoch undefined but sees the shared lease_state.max_epoch and checkpoint at (for example) epoch 10; it therefore passes #assertLeaseContinuity() and can renew, release, or validate a stale row at epoch 1 with its old token. acquire() also clears the process-local recovery epoch after one successful replacement, leaving stale rows for other items unfenced.
      • Remediation: Persist the recovery boundary in the shared SQLite lease state (or another durable recovery marker) and enforce it for every heartbeat, release, guard, and acquire across all repository instances and restarts. Do not clear a global recovery fence merely because one item was replaced. Add a regression using two repositories, a stale lease, explicit re-enrollment, and a reopened repository.
    • HIGH · Epoch-zero bootstrap aliases a historical lease store as emptysrc/adapters/sqlite/lease-witness.ts — bootstrapLocalLeaseWitness; src/adapters/sqlite/sqlite-repositories.ts — leases/lease_state schema and #assertLeaseContinuity()
      • Evidence: The remediation now installs a signed epoch-0 checkpoint in bootstrapLocalLeaseWitness(). SqliteRepositories always creates or retains lease_state.max_epoch = 0, while the leases table migration gives pre-fencing rows epoch = 0; the source comment explicitly says such existing operational stores are retained. There is no durable history/initialized marker. Consequently, a legacy database containing an active epoch-0 lease can be paired with the new epoch-0 witness: #assertLeaseContinuity() sees exact 0/0 equality, and heartbeat(), release(), and guard() do not reject a row at the current zero epoch. The old epoch-1 bootstrap would have mismatched the zero state and failed closed, so the new seed creates this compatibility hole.
      • Remediation: Add a durable lease-history/initialization marker and accept the epoch-0 bootstrap only when the database is provably new and has no prior lease history. Otherwise deny lease use or require explicit higher authenticated re-enrollment. Add a legacy-schema regression with an active epoch-0 row and verify heartbeat, guard, release, and acquire remain denied.

Failed groups

None.

@rapiercraft-forgedock

Copy link
Copy Markdown
Author

ForgeDock · Review Verdict

Run run_efaff56d-5576-4ac5-a942-54cf69a16aa5 · artifact art_e405c0a8-34a4-4b45-80bc-721b5243c582

Disposition: request_changes · Reviewed SHA: 11cdeeb102dd39cefb2ce795e17e845a2c39f4f5

Reviewer roles: correctness, data, concurrency

Review plan

Risk: high · Specialist group budget: 3
Identity: review-plan-07027de79084a0d686af · Generation: 1 · Frozen: yes
Absolute budget: 3 logical sessions (3 specialist groups), 2 attempts/group

Required capabilities

  • acceptance-correctness · score 1000 · mandatory evidence
  • concurrency · score 230 · mandatory evidence
  • data-integrity · score 100

Execution groups

  • review-correctness (correctness) · acceptance-correctness — mandatory intent, acceptance, target-authority, and correctness review
  • review-data (data) · data-integrity — Build Packet declares persistence/encoding/schema risk; diff changes persisted or interoperable data semantics: src/adapters/sqlite/sqlite-repositories.ts
  • review-concurrency (concurrency) · concurrency — scheduler, lease, lock, or queue path: src/adapters/sqlite/lease-witness.test.ts, src/adapters/sqlite/lease-witness.ts; Build Packet declares concurrency/distributed-coordination risk; diff changes concurrency or coordination semantics: src/adapters/sqlite/lease-witness.ts, src/adapters/sqlite/orchestration-admission.test.ts, src/adapters/sqlite/sqlite-repositories.test.ts, src/adapters/sqlite/sqlite-repositories.ts

Non-executing specialist roles

  • security · score 35 · below-threshold — diff changes security/trust semantics: src/adapters/sqlite/lease-witness.test.ts, src/adapters/sqlite/sqlite-repositories.test.ts
  • api-compatibility · score 0 · below-threshold — no qualifying evidence
  • frontend · score 0 · below-threshold — no qualifying evidence
  • infrastructure · score 0 · below-threshold — no qualifying evidence

Findings

  • HIGH · Epoch-zero bootstrap aliases a historical lease store as empty · BLOCKING · reviewers: concurrency
    The remediation now installs a signed epoch-0 checkpoint in bootstrapLocalLeaseWitness(). SqliteRepositories always creates or retains lease_state.max_epoch = 0, while the leases table migration gives pre-fencing rows epoch = 0; the source comment explicitly says such existing operational stores are retained. There is no durable history/initialized marker. Consequently, a legacy database containing an active epoch-0 lease can be paired with the new epoch-0 witness: #assertLeaseContinuity() sees exact 0/0 equality, and heartbeat(), release(), and guard() do not reject a row at the current zero epoch. The old epoch-1 bootstrap would have mismatched the zero state and failed closed, so the new seed creates this compatibility hole.
    Location: src/adapters/sqlite/lease-witness.ts — bootstrapLocalLeaseWitness; src/adapters/sqlite/sqlite-repositories.ts — leases/lease_state schema and #assertLeaseContinuity()
    Sources: concurrency:review-concurrency-epoch-zero-history-alias
    Sessions: 01a00947-1e4e-7f38-a6d2-f59a0af03ca9
    Remediation: Add a durable lease-history/initialization marker and accept the epoch-0 bootstrap only when the database is provably new and has no prior lease history. Otherwise deny lease use or require explicit higher authenticated re-enrollment. Add a legacy-schema regression with an active epoch-0 row and verify heartbeat, guard, release, and acquire remain denied.
  • HIGH · Explicit re-enrollment does not fence stale leases across repository instances · BLOCKING · reviewers: data, concurrency
    [concurrency:review-concurrency-recovery-fence-not-durable] SqliteRepositories.reEnroll() records the authenticated recovery boundary only in the private #recoveryEpoch. heartbeat(), release(), and guard() reject a pre-re-enrollment row only when that private field is set. A second SqliteRepositories using the same database, or a repository reopened after re-enrollment, has #recoveryEpoch undefined but sees the shared lease_state.max_epoch and checkpoint at (for example) epoch 10; it therefore passes #assertLeaseContinuity() and can renew, release, or validate a stale row at epoch 1 with its old token. acquire() also clears the process-local recovery epoch after one successful replacement, leaving stale rows for other items unfenced.

[data:review-data-reenrollment-fence-process-local] reEnroll stores the recovery epoch only in the instance field #recoveryEpoch. heartbeat, release, and guard reject an old lease row only while that field is set. acquire unconditionally sets #recoveryEpoch = undefined after any successful insertion, even when it acquires a different item and leaves older lease rows untouched. For example, leases A(epoch 1) and B(epoch 2) can exist, an authenticated re-enrollment can move the witness/local maximum to 10, and acquiring a new item C at epoch 11 clears the flag; the old token for A then passes continuity and the row.epoch &lt; #recoveryEpoch check is no longer performed. A separately opened repository has the same problem immediately because the recovery epoch is not persisted. Thus an old holder can heartbeat, pass its guard, or release after the explicit recovery that should fence it.
Location: src/adapters/sqlite/sqlite-repositories.ts — #recoveryEpoch, reEnroll(), acquire(), heartbeat(), release(), and guard()
Sources: data:review-data-reenrollment-fence-process-local, concurrency:review-concurrency-recovery-fence-not-durable
Sessions: 01a00947-1dd5-7a6c-b86e-e06daab8cffb, 01a00947-1e4e-7f38-a6d2-f59a0af03ca9
Remediation: [concurrency:review-concurrency-recovery-fence-not-durable] Persist the recovery boundary in the shared SQLite lease state (or another durable recovery marker) and enforce it for every heartbeat, release, guard, and acquire across all repository instances and restarts. Do not clear a global recovery fence merely because one item was replaced. Add a regression using two repositories, a stale lease, explicit re-enrollment, and a reopened repository.

[data:review-data-reenrollment-fence-process-local] Persist a recovery-fence epoch (or equivalent authenticated recovery marker) in the durable lease state and consult it for every heartbeat, guard, release, and acquire across repository instances and restarts. Do not clear the marker after the first unrelated successful acquisition; add a regression with two pre-existing lease rows, recovery, acquisition of a third item, repository reopen, and denial of all old tokens.

  • HIGH · Epoch-zero bootstrap does not prove the SQLite store is empty · reviewers: correctness
    bootstrapLocalLeaseWitness now installs a signed epoch-0 checkpoint (lease-witness.ts, writeInitialCheckpoint). SqliteRepositories creates/retains lease_state.max_epoch=0, and the leases table uses epoch INTEGER NOT NULL DEFAULT 0 (sqlite-repositories.ts, constructor). #assertLeaseContinuity only requires witness epoch equality with #localMaximum; it never checks whether leases already contains rows. Consequently, a pre-fencing database migrated by this constructor with an epoch-0 active or expired row is accepted as compatible with the freshly bootstrapped witness: heartbeat/release/guard can continue for an epoch-0 row, and acquire can delete an expired row and advance to epoch 1. The newly added file-backed tests create empty databases, so they do not exercise this legacy-history path.
    Location: src/adapters/sqlite/sqlite-repositories.ts — constructor lease schema; acquire; #assertLeaseContinuity
    Sources: correctness:review-correctness-epoch-zero-legacy-history
    Sessions: 01a00947-1e10-7338-8367-413ebe0c26e0
    Remediation: When the local maximum and verified witness are both epoch 0, require affirmative evidence that the lease store has no existing lease rows (and fail closed if legacy rows are present). Require an explicit higher authenticated reEnroll before recovery; the existing recovery epoch then fences the epoch-0 rows. Add a regression using a pre-populated/legacy epoch-0 lease row and assert acquire, heartbeat, release, and guard deny until re-enrollment.

Verification

Command Status Duration Summary
git diff --check passed 53 ms warning: in the working copy of 'src/adapters/sqlite/orchestration-admission.test.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/adapters/sqlite/sqlite-repositories.test.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/adapters/sqlite/sqlite-repositories.ts', LF will be replaced by CRLF the next time Git touches it
C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run build passed 9982 ms > forgedock@1.8.0 build | > tsc -p tsconfig.json
C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run docs:build passed 7165 ms - generating sitemap... | ✓ generating sitemap... | build complete in 5.30s.
C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js test passed 115873 ms # skipped 8 | # todo 0 | # duration_ms 82725.5009

@rapiercraft-forgedock

Copy link
Copy Markdown
Author

ForgeDock Review Evidence

One bounded projection for the complete frozen reviewer wave. The wave is incomplete; no partial approval was issued.

Completed groups

review-correctness · correctness · completed
  • Session lineage: 01a009fa-1d30-76bc-8b48-6e7bfc3e65ed
  • Summary: Found one blocking lease-fencing correctness defect: authenticated re-enrollment updates max_epoch before recovery_epoch and lease mutations are not atomic with the fence, so stale holders can remain usable across a crash or race.
  • Findings:
    • HIGH · Authenticated re-enrollment can leave stale lease rows usable after a crash or racesrc/adapters/sqlite/sqlite-repositories.ts — reEnroll(), #acceptWitness(), #assertLeaseRowUsable(), heartbeat(), and release()
      • Evidence: In src/adapters/sqlite/sqlite-repositories.ts, reEnroll() writes the new checkpoint, calls #acceptWitness(snapshot) (which autocommit-updates lease_state.max_epoch), and only afterward performs a separate UPDATE that records recovery_epoch (around lines 442-452). If the process exits or the second write fails in that interval, a store can persist max_epoch=10 and the checkpoint at epoch 10 while recovery_epoch remains NULL and an old lease row remains at epoch 1. After restart, exact witness/max equality succeeds; #assertLeaseRowUsable() only rejects the old row when a recovery marker exists (or for the special epoch-zero case), so the old holder can heartbeat or release. The same gap is reachable concurrently: a heartbeat/release can pass #assertLeaseRowUsable() before the recovery update and then execute its unconditional token-based UPDATE/DELETE after re-enrollment; those mutations do not predicate on the current recovery fence.
      • Remediation: Make the SQLite max_epoch and recovery_epoch transition one durable transaction, leaving the old state in place until the recovery fence is committed; if the checkpoint write cannot be paired with that transaction, persist a recovery-pending state that denies all lease operations rather than exposing the new maximum. Also make heartbeat/release (and other lease-row mutations) check the durable recovery epoch in the same transaction or as a conditional SQL predicate, so a row below the fence cannot be mutated after re-enrollment.
review-data · data · completed
  • Session lineage: 01a009fa-1d78-79c2-996f-923a9bd5ad16
  • Summary: The SQLite re-enrollment path still has a blocking durability race: it exposes the new max epoch before persisting the stale-row recovery fence.
  • Findings:
    • HIGH · Re-enrollment publishes max_epoch before its stale-lease fencesrc/adapters/sqlite/sqlite-repositories.ts — reEnroll(), #acceptWitness(), and #assertLeaseRowUsable()
      • Evidence: reEnroll() calls #acceptWitness(snapshot) before issuing the separate UPDATE lease_state ... recovery_epoch statement. #acceptWitness() durably raises max_epoch first, and reEnroll() has no enclosing inTransaction. If the process stops, the second update fails, or another repository operates between those statements, the database can contain max_epoch=10 and recovery_epoch=NULL while the signed witness is already at epoch 10. #assertLeaseRowUsable() then permits a normal historical row at epoch 1: it is not above max_epoch, the epoch-zero special case does not apply, and no recovery_epoch rejects it. Its old token can therefore heartbeat and remain usable after explicit re-enrollment.
      • Remediation: Persist the max epoch and recovery_epoch in one SQLite BEGIN IMMEDIATE transaction, without exposing the new max as a committed state until the recovery fence is committed with it. Keep witness publication ordered so a crash before the database commit leaves a witness/database mismatch and therefore fails closed. Add a regression that interrupts or interleaves a second repository between witness acceptance and fence recording, then verifies old-epoch heartbeat, guard, and release are denied after restart.

Failed groups

  • review-concurrency: No durable issue-level progress for 481s on run run_efaff56d-5576-4ac5-a942-54cf69a16aa5

Sign up for free to 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.

1 participant