-
Notifications
You must be signed in to change notification settings - Fork 1
feat(bus): add subscribeOnce helper (strictly decomposed from B-0459) #3775
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 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
aa8a424
feat(bus): add subscribeOnce helper (decomposed from B-0459)
AceHack 13c497b
fix(bus): remove unused Topic import and rename unused mock param (#3…
AceHack 5618091
fix(bus): remove existsSync to eliminate TOCTOU race (#3775)
AceHack c538a83
fix(bus): subscribeOnce — surface allowlist + ENOENT-aware seen read
AceHack 36b6a35
chore(bus): correct describe label B-0449 → B-0459 in subscribe.test.ts
AceHack 599a8c9
Merge remote-tracking branch 'origin/main' into otto-bg-3775-fix-writ…
AceHack 5b490ba
fix(bus): propagate seen-file write failures from subscribeOnce
AceHack 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
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,69 @@ | ||
| import { describe, expect, test, mock } from "bun:test"; | ||
| import { subscribeOnce } from "./subscribe"; | ||
| import type { MessageEnvelope, Topic } from "./types"; | ||
| import { rmSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { BUS_DIR } from "./bus"; | ||
|
|
||
|
AceHack marked this conversation as resolved.
AceHack marked this conversation as resolved.
|
||
| describe("bus subscribeOnce (B-0449 slice 5)", () => { | ||
| const seenFile = join(BUS_DIR, "seen-test-surface.json"); | ||
|
AceHack marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Helper to clear state | ||
| function clearState() { | ||
| try { rmSync(seenFile); } catch {} | ||
| } | ||
|
|
||
| test("calls handler for unseen envelopes and records seen state", async () => { | ||
| clearState(); | ||
|
|
||
| const env1: MessageEnvelope = { | ||
| id: "env-1", | ||
| from: "otto", | ||
| to: "test-surface" as any, | ||
| timestamp: new Date().toISOString(), | ||
| expiresAt: new Date(Date.now() + 10000).toISOString(), | ||
| topic: "work-assignment", | ||
| payload: { rowId: "B-1234", priority: "P1", rationale: "test" }, | ||
| }; | ||
|
|
||
| const fakeList = mock(() => { | ||
| return [env1]; | ||
| }); | ||
|
|
||
| const handler = mock(async (env) => {}); | ||
|
|
||
| await subscribeOnce("work-assignment", "test-surface", handler, { list: fakeList as any }); | ||
|
|
||
| expect(handler).toHaveBeenCalledTimes(1); | ||
| expect(handler).toHaveBeenCalledWith(env1); | ||
|
|
||
| // Call again, should not trigger handler because it was recorded in seen-test-surface.json | ||
| await subscribeOnce("work-assignment", "test-surface", handler, { list: fakeList as any }); | ||
| expect(handler).toHaveBeenCalledTimes(1); // Still 1 | ||
| }); | ||
|
|
||
| test("does not mark as seen if handler throws", async () => { | ||
| clearState(); | ||
|
|
||
| const env2: MessageEnvelope = { | ||
| id: "env-2", | ||
| from: "otto", | ||
| to: "test-surface" as any, | ||
| timestamp: new Date().toISOString(), | ||
| expiresAt: new Date(Date.now() + 10000).toISOString(), | ||
| topic: "work-assignment", | ||
| payload: { rowId: "B-2222", priority: "P2", rationale: "test2" }, | ||
| }; | ||
|
|
||
| const fakeList = mock(() => [env2]); | ||
| const handlerFailing = mock(async () => { throw new Error("fail"); }); | ||
|
|
||
| await subscribeOnce("work-assignment", "test-surface", handlerFailing, { list: fakeList as any }); | ||
|
|
||
| expect(handlerFailing).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Call again, should retry because it failed and wasn't marked seen | ||
| await subscribeOnce("work-assignment", "test-surface", handlerFailing, { list: fakeList as any }); | ||
| expect(handlerFailing).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); | ||
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,57 @@ | ||
| import { join } from "node:path"; | ||
| import { existsSync, readFileSync, writeFileSync } from "node:fs"; | ||
| import { BUS_DIR, ensureDir, list } from "./bus"; | ||
| import type { MessageEnvelope, Topic } from "./types"; | ||
|
AceHack marked this conversation as resolved.
AceHack marked this conversation as resolved.
AceHack marked this conversation as resolved.
|
||
|
|
||
|
AceHack marked this conversation as resolved.
AceHack marked this conversation as resolved.
AceHack marked this conversation as resolved.
|
||
| /** | ||
| * Reads envelopes from the bus matching the given topic and recipient, | ||
| * calls the handler for each unseen envelope, and marks them as seen | ||
| * in a surface-specific seen.json file. | ||
| */ | ||
| export async function subscribeOnce<T extends Topic>( | ||
| topic: T, | ||
| surface: string, | ||
| handler: (envelope: MessageEnvelope & { topic: T }) => Promise<void> | void, | ||
| adapters = { list } | ||
| ): Promise<void> { | ||
| ensureDir(); | ||
| const seenFile = join(BUS_DIR, `seen-${surface}.json`); | ||
| let seenIds: Set<string>; | ||
|
AceHack marked this conversation as resolved.
|
||
|
|
||
| try { | ||
| if (existsSync(seenFile)) { | ||
| const data = JSON.parse(readFileSync(seenFile, "utf8")); | ||
| seenIds = new Set(Array.isArray(data) ? data : []); | ||
| } else { | ||
| seenIds = new Set(); | ||
| } | ||
| } catch { | ||
| seenIds = new Set(); | ||
| } | ||
|
|
||
|
AceHack marked this conversation as resolved.
|
||
| // Get all envelopes matching topic and targeted at this surface (or broadcast) | ||
| const envelopes = adapters.list({ topic, to: surface as any }); | ||
|
|
||
| let newlySeen = false; | ||
|
|
||
| for (const envelope of envelopes) { | ||
| if (!seenIds.has(envelope.id)) { | ||
| try { | ||
| await handler(envelope as MessageEnvelope & { topic: T }); | ||
| seenIds.add(envelope.id); | ||
| newlySeen = true; | ||
| } catch (err) { | ||
| // If handler fails, we do NOT mark as seen, so it can be retried next tick | ||
| console.error(`[subscribeOnce] Handler for ${envelope.id} failed:`, err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (newlySeen) { | ||
| try { | ||
| writeFileSync(seenFile, JSON.stringify(Array.from(seenIds), null, 2)); | ||
Check failureCode scanning / CodeQL Potential file system race condition High
The file may have changed since it
was checked Error loading related location Loading |
||
|
AceHack marked this conversation as resolved.
Fixed
|
||
| } catch (err) { | ||
| console.error(`[subscribeOnce] Failed to write seen file:`, err); | ||
|
AceHack marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
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.
Uh oh!
There was an error while loading. Please reload this page.