fix(api): persist plan store to disk, add expiry and 410 on expired plans - #24
Merged
Merged
Conversation
…lans - PlanStore class follows swapStore file-per-record pattern with atomic writes - Plans persist across restarts and are visible across processes via shared SWAP_STORE_DIR - DEFAULT_PLAN_TTL_MS = 30 minutes; expired plans cleaned up on startup and via cleanup() - execute endpoint returns 410 Gone (plan_expired) vs 404 (plan_not_found) - SWAP_STORE_DIR='' keeps everything in memory for tests - get() deliberately does not delete an expired plan: dropping it there would make the following execute answer 404 where it must answer 410 - 11 new tests: survives reload, cross-process visibility, expiry path (in memory and after restart), unknown-id path, memory-only mode, cleanup, and the 410-vs-404 distinction at the HTTP boundary Closes Micopay#17
ericmt-98
approved these changes
Aug 24, 2026
ericmt-98
left a comment
Contributor
There was a problem hiding this comment.
This is the reference implementation for how to take one of these issues. Merging.
What stood out:
- You reused the mechanism instead of inventing one. BRIDGE-10 asked you to follow
swapStore's file-per-record + atomic-write pattern and itsSWAP_STORE_DIRdisable switch rather than building a second store. You did, down to the per-process.tmpsuffix, and theplan-prefix is a real answer to the shared-directory problem rather than a hope. - The tests are tests. Instantiating
PlanStoreagainst amkdtempdirectory and re-importing the module withvi.resetModules()makes "survives a restart" and "a second process sees it" genuine assertions rather than mocks agreeing with themselves. You also rantest:concurrency, which the issue's notes pointed at. - The design note earned its place.
get()not deleting an expired record, because theisExpired()check right after it would then answer 404 where it owes a 410, is the subtle part of this issue — and you found it, explained it, and left the reasoning in the code where the next person will need it.
All five acceptance criteria met.
Three follow-up nits, none blocking, no need to hold the merge for them:
set()keys memory byplanIdwhileloadFromDisk()re-keys byrecord.plan.id. Identical today; if they ever diverge the key changes across a restart.archivoDePlan()sanitises the id, soa/banda_bcollide on one file. Plan ids are internally generated, so this isn't reachable now.cleanup()only runs at boot, so a long-lived process never reaps. A periodic call would close it.
Thank you — this was a pleasure to review.
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #17
Description
Migrates
planStorefrom a bareMap<string, SwapPlan>to a disk-backedPlanStoreclass following the same file-per-record + atomic-write pattern already used byswapStorein the same file. Plans now survive restarts and are visible across processes sharingSWAP_STORE_DIR. Adds an explicit TTL (30 min default) and distinguishes expired plans (410 Gone) from unknown plan IDs (404 Not Found).Changes
apps/api/src/lib/swapStore.ts—PlanStoreclass replaces the bareMap; theplanStoreexport is preserved, so existingset/getcall sites are unchanged. Plans are written asplan-<id>.jsonunder the sameSTORE_DIRthe swap store uses; the prefix is what keeps the two from reading each other's files.apps/api/src/routes/agent.ts— the execute endpoint now answers 410plan_expiredvs 404plan_not_found. (The plan/execute routes live inagent.ts, notswaps.ts.)apps/api/src/index.ts—planStore.cleanup()on startup, so the directory does not grow forever.apps/api/src/__tests__/plan-store.test.ts— 8 store-level tests.apps/api/src/__tests__/agent-execute-plan-expiry.test.ts— 3 route-level tests for the 410/404 split.One deliberate design note
get()does not delete an expired plan. If it did, theisExpired()check that runs immediately after it inexecutewould find nothing and answer 404 — exactly the confusion this issue is about. Expired records are reaped bycleanup()instead. For the same reasonloadFromDisk()skips expired records but leaves their files in place, andisExpired()reads the file directly so the 410 survives a restart.get()also falls back to disk on a memory miss, which is what makes the second-process case work for an instance that was already running when the plan was created.Acceptance Criteria
SWAP_STORE_DIRsee each other's plansplan_id404SWAP_STORE_DIR=''keeps everything in memoryVerification
npm test -w @micopay/api— 23 files, 163 passed, 1 skipped (was 21 files / 152 passed before this change)npm run test:concurrency -w @micopay/api— 2/2 OK, unchangednpm run typecheck -w @micopay/api— cleanNote:
typecheckfails on a clean checkout withCannot find module '@micopay/types'until the workspace packages are built (npx turbo build --filter=@micopay/types --filter=@micopay/sdk). That is pre-existing and unrelated to this change.Type of Change
Checklist