Skip to content

Commit 1a7ad60

Browse files
pmaxhoganclaude
andcommitted
docs(m7): record recheck-2 fixes (R2-P1-1..R2-P2-4); round-3 ran, M7 closes after recheck-3
User-approved round-3 fixed all 6 recheck-2 findings (past the normal cap-2). Documents the keyset pagination root fix, the no-early-stop lag reconcile, the streamed per-op activity (+ the in-run placement that avoids the single-connection-pool deadlock found via 4 chaos timeouts), the timestamp validation, the i64 wrap fix, and the localized filter dropdown. M7 closes after codex recheck-3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CyiRqk2DVwmJjEu5gcD1m
1 parent 8467bd1 commit 1a7ad60

1 file changed

Lines changed: 90 additions & 0 deletions

File tree

design/CODEX_NOTES.md

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,3 +1069,93 @@ None. All 2 P1 + 4 P2 are fully fixed with exercising tests. The R1-P2-1 debounc
10691069
(750ms) means the header aggregate can lag a live upload by up to that window -
10701070
intentional (coalesces a burst into one query); the live tail itself is still
10711071
sub-500ms via `activity:new`.
1072+
1073+
## M7 codex recheck-2 fixes (round 3: 2 P1 + 4 P2, USER-APPROVED past the cap-2)
1074+
1075+
The codex recheck-2 (`.claude/codex-reviews/M7-recheck2-20260624-140630.md`,
1076+
baseline f9fb164, M7 @ 1da5b59; CI + Chaos GREEN on 3 OS) raised 2 P1 + 4 P2 -
1077+
all activity-dashboard correctness/validation (not data-safety). The user
1078+
explicitly approved a round-3 to fix ALL SIX (past the normal cap-2, analogous to
1079+
the M6 exception); codex recheck-3 runs next and M7 closes after it. No spec
1080+
deviations. Fix spec: `.claude/m7-codex-fix-spec-r3.md`.
1081+
1082+
- **R2-P1-2 (offset pagination over a live-prepended table) -> KEYSET.** The two
1083+
P1s share one root: `query_activity` was OFFSET-based over `activity_log`, which
1084+
is actively PREPENDED to, so rows inserted between `loadInitial` and `loadMore`
1085+
shifted every later page (skip/underload while still advancing). Fix: switched
1086+
`query_activity` to KEYSET pagination. `state::PageRequest` is now a `(before_ts,
1087+
before_id)` cursor + `limit` (with `::first(limit)` / `::after_cursor(ts,id,
1088+
limit)` ctors); the SQL pages `WHERE ... AND (?6 IS NULL OR ts < ?6 OR (ts = ?6
1089+
AND id < ?7)) ORDER BY ts DESC, id DESC LIMIT n` so ties on `ts` are stable.
1090+
`ActivityPage` gained `has_more` (a full page MAY have more older rows). The DTOs
1091+
(`PageRequestDto` -> `beforeTs`/`beforeId`, `ActivityPageDto` -> `nextBeforeTs`/
1092+
`nextBeforeId`/`hasMore`, dropped `page`) + `ipc/types.ts` + the store + the
1093+
Activity.vue caller were updated in the same pass. The store carries the oldest
1094+
loaded `(ts,id)` as `oldestCursor` and pages by it. One sqlx re-prepare (0 drift:
1095+
one offset query removed, one keyset query added). Tests: `sqlite.rs`
1096+
`query_activity_keyset_is_stable_under_inserts` (newer prepended rows never shift
1097+
an older keyset page) + the rewritten `..._paginates_correctly` (cursor walk);
1098+
store `loadMore pages by the oldest (ts,id) CURSOR`.
1099+
1100+
- **R2-P1-1 (lag reconcile early-stops before the gap is covered).** The reconcile
1101+
broke on `recoveredThisPage === 0`, which could stop at an already-seen newest
1102+
page while the dropped rows sat DEEPER (a ring-buffer broadcast evicts the OLDEST
1103+
of a burst, so dropped rows are below the latest delivered). Fix: the reconcile
1104+
now walks by the keyset cursor over a bounded SCAN BUDGET (`min(LIVE_TAIL_CAP,
1105+
max(PAGE_SIZE, skipped + PAGE_SIZE))` rows) and does NOT stop on a zero-new page;
1106+
it stops only on history-exhausted or budget-spent. Recovered rows can be older
1107+
than rows already in the tail, so they are merged in SORTED (newest-first) order
1108+
via the new `mergeRecoveredLive` (not blindly prepended), then capped. Tests:
1109+
store `recovers DEEPER dropped rows even when the newest page is already seen`
1110+
(the exact recheck-2 P1 shape) + the existing multi-page recovery test.
1111+
1112+
- **R2-P2-1 (per-op activity lost on a mid-plan crash).** Successful upload/trash
1113+
activity was written in a POST-PASS after `executor.execute()` finished the whole
1114+
source plan, so a crash/shutdown mid-plan lost the audit rows + byte aggregates,
1115+
and a large initial backup showed no per-file activity until completion. Fix:
1116+
streamed per-op activity. `Executor::execute` takes a new `on_outcome:
1117+
&OutcomeSink<'_>` (a boxed-future per-op sink); the DefaultExecutor invokes it
1118+
INSIDE `ExecOne::run` right after the op's durable commit (and BEFORE returning),
1119+
so the activity DB write runs as part of the in-flight future polled by the
1120+
FuturesUnordered drain loop. (Doing it in the drain loop's select arm instead
1121+
deadlocked the single-connection pool against a concurrent op holding the
1122+
connection - found via 4 chaos tests timing out at 120s; the in-`run` placement
1123+
fixes it.) The orchestrator's `on_outcome` builds the `NewActivity` synchronously
1124+
from the borrowed outcome and the future borrows only `self` (satisfies the sink
1125+
bound). Routes through `record_activity` so `activity:new` still broadcasts per
1126+
op. `noop_outcome_sink` is exposed for the chaos harness + tests. Test:
1127+
`orchestrator.rs` `per_op_activity_survives_a_mid_plan_stop` (RecordingExecutor
1128+
streams N outcomes then errors; the committed op's row persists).
1129+
1130+
- **R2-P2-2 (unbounded `before_ts` wipes the log).** `clear_activity_older_than`
1131+
accepted any `before_ts` (an `i64::MAX` prunes to the hard cap). Fix: a shared
1132+
`validate_timestamp_bound` (>= 0 and <= now + 1 day, stable
1133+
`internal.invalid_input`) gates `clear_activity_older_than` AND the
1134+
`query_activity` `sinceMs`/`beforeMs` filters AND the keyset `beforeTs`. Tests:
1135+
`activity.rs` `validate_timestamp_bound_rejects_out_of_range`,
1136+
`validate_filter_rejects_out_of_range_time_filters`,
1137+
`validate_page_rejects_out_of_range_before_ts`.
1138+
1139+
- **R2-P2-3 (silent u64<->i64 wrap on counts).** `activity_log.file_count` /
1140+
`bytes` were cast `u64 -> i64` and back with `as`, so a value > `i64::MAX` wrapped
1141+
negative and summaries clamped to 0. Fix: `write_activity` uses `i64::try_from`
1142+
and REJECTS an over-range value (`internal.bad_request`); the read path decodes
1143+
via a new `decode_nonneg_u64` (`u64::try_from`, rejects a negative stored value).
1144+
Test: `sqlite.rs` `write_activity_rejects_counts_above_i64_max` (over-cap
1145+
rejected, `i64::MAX` boundary round-trips).
1146+
1147+
- **R2-P2-4 (raw event codes in the filter dropdown).** The event-type filter
1148+
rendered raw backend codes (`{{ et }}`) while the table localized them. Fix: the
1149+
dropdown option text now uses the shared `activityEventLabel` helper (via
1150+
Activity.vue's `eventLabel(et)`), keeping the raw code as the option value +
1151+
`title`. Test: `activity-filter-dropdown.test.ts` (mounts Activity.vue, asserts a
1152+
known code renders its localized label while value/title stay the raw code, and
1153+
an unknown code falls back to the raw code).
1154+
1155+
### Residual / not-fixed (recheck-2)
1156+
None. All 2 P1 + 4 P2 are fully fixed with exercising tests. Keyset `has_more` is a
1157+
conservative "a full page MAY have more" (an exactly-full final page costs one
1158+
extra empty fetch, never a skipped row). The per-op activity sink runs one write
1159+
per op (correctness: no mid-plan audit loss + per-file visibility); DESIGN s18.4's
1160+
1-second batched activity writer remains a future optimization, not required for
1161+
the correctness fix.

0 commit comments

Comments
 (0)