-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(quota): repair Windows reset fixtures and route inventory #3610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
02c941d
docs(windows): lock post-merge quota and cancellation repair roadmap
invalid-email-address 0db639a
fix(quota): complete reset route metadata and portable regression fix…
invalid-email-address e57b554
docs(windows): record quota repair and fault-injection evidence
invalid-email-address afdd38f
docs(windows): align quota closeout with exact-head check receipt
invalid-email-address 225ca85
Merge dev and reconcile the concurrent quota fixture repair
invalid-email-address File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
32 changes: 32 additions & 0 deletions
32
devlog/_plan/260905_windows_suite_stabilization/000_plan.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
devlog/_plan/260905_windows_suite_stabilization/009_1_postmerge_failures.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # 009.1 — Post-merge Windows evidence | ||
|
|
||
| ## Baselines | ||
|
|
||
| - Original stack: `293f3e675`, two all-green Windows six-shard runs | ||
| `33936695508` and `33937730205`. | ||
| - Merged stack: `3c920af5f`, run `33940032334`, job `101236063494`: | ||
| `server-auth.test.ts:4145` expected 499/client_cancel, received | ||
| 502/terminal, synthetic, mid_stream, streamAborted=true. Negative upstream | ||
| reset twin passed. The pending macOS jobs were cancelled by the parent after | ||
| the user excluded macOS; a subsequent single-job retry was cancelled by an | ||
| unrelated dev push, so that retry is no evidence. | ||
| - Pinned later dev: `593978db0`, run `33941712300`, isolated branch | ||
| `codex/win-dispatch-593978db0` so further dev pushes cannot cancel it. | ||
| Job `101240599941` (1/6) failed the real-second-process quota claim assertion | ||
| (empty stdout, expected true) and the hard-ceiling test (99.26 s against | ||
| 60 s). Job `101240599984` (6/6) repeated caller-cancel 502 and failed the | ||
| cold quota burst child (exit 1, expected 0). | ||
| - Job `101240600060` (3/6) adds three reconciliation failures: missing | ||
| GET /api/quota-resets declaration and an unresolved lazy dispatcher wrapper. | ||
| The handler and CLI verb already exist; these are integration inventory gaps. | ||
| - Job `101240599990` (5/6) fails quota-reset-notify.test.ts:515 because its | ||
| activation fixture configures HTTP despite the HTTPS-only schema. The local | ||
| focused check independently reproduces the same warning and assertion. | ||
| Full pinned Windows baseline: shards2/4 pass; 1/3/5/6 fail, eight assertions. | ||
|
|
||
| ## Hypotheses and falsifiers | ||
|
|
||
| Quota child H1: file URL `.pathname` is passed as a native script/import path. | ||
| Both call sites contain that conversion. Falsifier: stderr proves module loading | ||
| succeeded and failure occurred later. Child stdout alone cannot establish this. | ||
| H2: PATH selected a different Bun; pin process.execPath and observe stderr/exit. | ||
| H3: persistence failure; the claim API prints false on a caught write failure, | ||
| not empty output, so this does not explain the observed first-child signature. | ||
| Existing corpus case `env-paths/file-url-pathname-drive-slash.md` covers the | ||
| conversion and diagnostics gap; no duplicate case is needed. | ||
|
|
||
| Ceiling H1: fixture construction performs excessive real persistence. The first | ||
| 1024 claims persist; the remaining 976 newcomers are immediately evicted before | ||
| persistence. H2: pruning is intrinsically slow; a near-boundary fixture with the | ||
| same eviction still falsifies that. H3: an unrelated child stall dominates; | ||
| constant-write timing will distinguish it. Do not label this measured fsync | ||
| overhead: the inspected atomic writer uses synchronous persistence and Windows | ||
| ACL subprocesses, and exact per-operation time has not been measured. | ||
|
|
||
| Cancellation H1: Windows forced rewrite selects eager despite legacy-tee, and | ||
| eager lacks caller-cancellation provenance. `core.ts` passes caller abort to the | ||
| fetch controller but gives eager a separate turn controller; the link is one-way. | ||
| H2: transport fails before the caller signal is observed; requires event-order | ||
| evidence. H3: shared fixture/log contamination; weakened by request-ID filtering, | ||
| fresh harness logs and two repeated Windows failures. A deterministic reader | ||
| rejection/caller-signal test distinguishes the missing provenance from an actual | ||
| upstream reset. Do not weaken the 502 negative twin or the Windows eager safety | ||
| override. The old stack did not include this regression test (#3541). | ||
|
|
||
| ## Boundaries | ||
|
|
||
| These are reliability/test-portability findings, not credential-bypass findings. | ||
| No unshipped security investigation belongs here. Runtime security boundaries, | ||
| workflow permissions, Bun version, shard count and timeout ceilings remain fixed. |
193 changes: 193 additions & 0 deletions
193
devlog/_plan/260905_windows_suite_stabilization/100_quota_test_boundaries.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| # 100 — Portable quota child processes and bounded fixture setup | ||
|
|
||
| Class C3 after the quota route-registration finding; spec-satisfaction repair. | ||
| Trigger: run 33941712300 jobs 101240599941 | ||
| and 101240599984. Goal: preserve cold-process/durable-restart and hard-cap | ||
| assertions on Windows. Non-goals: production store changes, larger timeouts, | ||
| skips, retries, ACL bypasses. Owner: main; agents read-only unless the plan is | ||
| amended. Stop on contrary child stderr or changed store semantics and re-plan. | ||
|
|
||
| ## MODIFY tests/usage/quota-reset-seen-store.test.ts | ||
|
|
||
| At the real-second-process test, preserve the full file URL for dynamic import: | ||
|
|
||
| ```diff | ||
| -const storeUrl = new URL("../../src/quota/reset-seen-store.ts", import.meta.url).pathname; | ||
| +const storeUrl = new URL("../../src/quota/reset-seen-store.ts", import.meta.url).href; | ||
| -const proc = Bun.spawn(["bun", script], { | ||
| +const proc = Bun.spawn([process.execPath, script], { | ||
| ``` | ||
|
|
||
| The corpus's dynamic-import-needs-file-url case refines the initial proposal: | ||
| an import specifier stays a URL; only a spawn argv script becomes fileURLToPath. | ||
| Keep JSON.stringify around the generated import URL. Replace stdout-only wait | ||
| with Promise.all of proc.exited, stdout.text and stderr.text; assert exitCode=0 | ||
| with stdout/stderr in the assertion message, then return trimmed stdout. Keep | ||
| the sequential true/false assertions and OPENCODEX_HOME unchanged. | ||
|
|
||
| Replace the 2000-call hard-ceiling setup with this boundary probe (no mock): | ||
|
|
||
| ```ts | ||
| const now = Date.now(); | ||
| const future = now + 365 * DAY; | ||
| const path = join(getConfigDir(), "quota-reset-state.json"); | ||
| const seeded = Object.fromEntries(Array.from({ length: 1_023 }, (_, index) => [ | ||
| `live-${index}`, { at: now, resetAt: future + index }, | ||
| ])); | ||
| writeFileSync(path, JSON.stringify({ version: 1, claims: seeded, events: [] })); | ||
| resetQuotaResetStoreForTests(); | ||
| expect(claimCountForTests()).toBe(1_023); | ||
| expect(claimQuotaReset("boundary", now, future + 1_023)).toBe(true); | ||
| expect(claimCountForTests()).toBe(1_024); | ||
| expect(claimQuotaReset("nearer", now, future - 1)).toBe(true); | ||
| expect(claimCountForTests()).toBe(1_024); | ||
| expect(hasSeenQuotaReset("boundary")).toBe(false); | ||
| const expected = { ...seeded, nearer: { at: now, resetAt: future - 1 } }; | ||
| expect(JSON.parse(readFileSync(path, "utf8")).claims).toEqual(expected); | ||
| expect(claimQuotaReset("furthest", now, future + 2_000)).toBe(false); | ||
| expect(hasSeenQuotaReset("furthest")).toBe(false); | ||
| expect(claimCountForTests()).toBe(1_024); | ||
| expect(JSON.parse(readFileSync(path, "utf8")).claims).toEqual(expected); | ||
| resetQuotaResetStoreForTests(); | ||
| expect(claimCountForTests()).toBe(1_024); | ||
| expect(hasSeenQuotaReset("nearer")).toBe(true); | ||
| expect(hasSeenQuotaReset("boundary")).toBe(false); | ||
| expect(hasSeenQuotaReset("furthest")).toBe(false); | ||
| ``` | ||
|
|
||
| Hydration does not prune. Only a real insertion crosses 1024; the future dates | ||
| exclude age/settled pruning. Disabling insertion's prune must fail at 1025. | ||
| Disk equality and rehydration prove the retained claim is persisted, not merely | ||
| left in memory. This replaces 1024 setup writes with two production writes. | ||
|
|
||
| ## MODIFY tests/usage/quota-reset-observation.test.ts | ||
|
|
||
| Add fileURLToPath import, wrap the existing helper URL with it, and spawn with | ||
| process.execPath. Collect exit/stdout/stderr concurrently and include stderr in | ||
| the zero-exit assertion. Keep the fresh child home and empty-event assertion. | ||
| Clean that private temp home only after the child exits, using the existing | ||
| test cleanup helper if teardown is added. No helper source change. | ||
|
|
||
| ## Acceptance and verification | ||
|
|
||
| - Focused command: `bun test tests/usage/quota-reset-seen-store.test.ts tests/usage/quota-reset-observation.test.ts`. | ||
| - Typecheck: `bun run typecheck`. No local full suite. | ||
| - Original Windows red is captured in 009.1. Final integration uses existing | ||
| ci.yml workflow_dispatch lane=all on a fixed task branch, never a moving dev ref. | ||
| - Mutant: temporarily omit claimQuotaReset's prune call; run only the hard-cap | ||
| case, require failure at 1025, then restore source exactly. This is a local | ||
| focused test, not a full suite. No mutant is committed or pushed. | ||
| - The initial test-only slice leaves store/schema untouched; the amendment below | ||
| also repairs existing route/capability inventories and their generated reference. Existing | ||
| corpus case covers the path issue; add this occurrence only after Windows proof. | ||
| - Verifiers name direct files and the production prune owner. CI commands were | ||
| observed in the baseline logs; local focused command is executed during B/C. | ||
|
|
||
| ## Quota integration inventory amendment (same newly merged feature) | ||
|
|
||
| Windows job101240600060 also fails management-route-registry reconciliation: | ||
| GET /api/quota-resets is absent; the dispatcher wrapper is unresolved. This is | ||
| platform-independent integration debt introduced with quota reset, not an OS | ||
| timing defect. The route and CLI implementation already exist. Extend wp8's | ||
| scope to the following four metadata/dispatch/derived-reference files; no | ||
| store, authentication, authorization or handler behavior changes. | ||
|
|
||
| MODIFY `src/server/management/route-registry.ts`: add beside the negated routes: | ||
|
|
||
| ```ts | ||
| { method: "GET", path: "/api/quota-resets", module: "server/management/quota-reset-routes", mutates: false, mechanism: "negated-guard" }, | ||
| ``` | ||
|
|
||
| MODIFY `src/server/management-api.ts`: use the existing lazy namespace mount | ||
| pattern (routing profiles and Lab use the same helper): | ||
|
|
||
| ```diff | ||
| -if (ctx.url.pathname !== "/api/quota-resets") return null; | ||
| +if (!pathInManagementNamespace(ctx.url.pathname, "/api/quota-resets")) return null; | ||
| ``` | ||
|
|
||
| The real handler keeps exact path and GET guards. Child paths now import that | ||
| handler before falling through; prefix collisions still do not load it. This | ||
| small lazy-load scope change is explicit, not disguised as no behavior change. | ||
| No route scanner exemption, duplicate owner entry, or assumed method is added. | ||
|
|
||
| MODIFY `src/cli/capabilities.ts`: declare the already-implemented command: | ||
|
|
||
| ```ts | ||
| { | ||
| command: ["provider", "resets"], | ||
| summary: "Show recently detected quota resets.", | ||
| routes: [{ method: "GET", path: "/api/quota-resets" }], | ||
| flags: [ | ||
| { name: "--limit", value: "number", summary: "Maximum events to return." }, | ||
| { name: "--json", value: "boolean", summary: "Emit the API payload as JSON." }, | ||
| ], | ||
| mutates: false, | ||
| json: "payload", | ||
| }, | ||
| ``` | ||
|
|
||
| MODIFY `skills/ocx/references/01_management_surface.md` mechanically via | ||
| `bun run skill:surface`, which renders the capability registry. No new command | ||
| implementation and no CLI-parity exemption: provider-runtime.ts already sends | ||
| the request. The value chain is declaration -> capability consumers and surface | ||
| renderer -> generated Markdown checked by skill-ocx.test.ts; no new type/enum. | ||
|
|
||
| Extra focused verification: management-route-registry.test.ts, | ||
| cli-capabilities.test.ts, skill-ocx.test.ts, quota-reset-notify.test.ts, | ||
| quota-reset-core-boundary.test.ts. Check exact GET, invalid limit, non-GET, | ||
| child path, prefix collision and lazy core boundary. Audit the dispatch diff | ||
| explicitly for auth bypass/import exposure; no workflow changes are planned. | ||
|
|
||
| ## Roadmap audit | ||
|
|
||
| Independent gpt-6-astra/high reviewer: VERDICT PASS; no blocking issues. Auth | ||
| precedes dispatch; child/prefix fallthrough and the inert registry stay intact. | ||
| Main baseline focused registry+capability check: 27 pass / 3 fail (registry | ||
| reconciliation only), exit 1, matching Windows. This approves the design, not | ||
| implementation. The two superseded scope descriptions were synchronized. | ||
|
|
||
| ## B-phase evidence amendment: activation fixture (one more quota test) | ||
|
|
||
| Final Windows job101240599990 and the local seven-file check both fail | ||
| quota-reset-notify.test.ts:515: activation expected true, actual false. The | ||
| config warning names webhookUrl. H1 is confirmed by the fixture's http URL | ||
| against config.ts's https-only schema. H2 (stale cache) is contradicted by the | ||
| explicit cache reset; H3 (network receiver failure) cannot explain failure | ||
| before activation/delivery. Do not change the schema or TLS validation. | ||
|
|
||
| MODIFY only that test's fixture: configure a reserved HTTPS URL | ||
| `https://hooks.example.test/activation`; wrap the existing fetch function in | ||
| the test to map exactly that URL to its already-existing loopback HTTP server. | ||
| Preserve method, body, headers, redirect and signal; other URLs delegate unchanged. | ||
| Record the requested HTTPS URL and assert one call. Restore fetch in finally. | ||
| The test still proves config -> activation -> quota writer -> actual HTTP body; | ||
| it deliberately does not claim TLS integration. Lower-level policy tests remain. | ||
| Replace the 40x25ms body polling with a completion promise resolved by the real | ||
| receiver. Implementation review caught that an outer test timeout does not | ||
| unwind an indefinitely awaited promise: race the receiver against the existing | ||
| INTERNAL_DEADLINE_MS, clear its timer in finally, and use SERVER_BUDGET_MS for | ||
| the real-server case. These are nested failure bounds, not polling sleeps. | ||
|
|
||
| Verification: rerun the original failing activation case, then the seven focused | ||
| files. The schema remains HTTPS-only; no fixture-only exception enters runtime. | ||
|
|
||
| ## wp8 closeout | ||
|
|
||
| Implemented at `0db639aea`. Seven focused files: 110 pass, 0 fail, 476 assertions | ||
| (4.20 s). Typecheck exit0; privacy scan passed. Hard-cap prune mutant: expected | ||
| 1024, actual1025 (exit1); restored source exactly, then 1pass/15assertions. | ||
| Missing-webhook fault: a transport stub withheld delivery with a 10ms watchdog; | ||
| the test rejected with `quota webhook was not received` and exited1 in126ms, | ||
| not an outer-timeout hang. Restored real transport and normal named deadline: | ||
| 1pass/9assertions. Neither fault mutation was committed. | ||
|
|
||
| Independent implementation review: PASS after adding the bounded receiver wait | ||
| and moving the fetch override into try/finally. Original route guards and store | ||
| implementation remain unchanged. Windows integration remains open under wp9/c-6; | ||
| these local focused checks are not claimed as Windows proof. | ||
|
|
||
| Receipt-binding correction: the closeout documentation commit changed HEAD after | ||
| the privacy receipt, so D correctly refused it. Re-audited the docs-only delta | ||
| (PASS, implementation unchanged); recapture a check receipt after this final | ||
| documentation commit before closing wp8. No failed gate is recorded as success. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the legacy work phases as superseded.
Lines 18-26 define
wp7,wp8, andwp9, withwp9dependent onwp8. Lines 78-89 still present the olderwp-argvandwp-cwdplan as active text. A reader can follow the stale write sets and omit the current quota and eager-cancellation work. Add an explicitHistorical — supersededheading to the old section or remove it.🤖 Prompt for AI Agents