Move run-derived state into per-user RunLog - #1114
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughRunLog now stores workflow projections, job observability, package success records, and activation milestones. Workflow operations use RunLog with D1 compatibility mirroring. Job views hydrate observability data, and account exports page all RunLog state. ChangesRunLog state and exports
Workflow projection migration
Job observability ownership
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant JobService
participant RunLog
participant JobView
JobService->>RunLog: persist terminal job observability
JobService->>RunLog: hydrate job view
RunLog-->>JobView: return status, errors, duration, and counters
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-1114.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/worker/src/jobs/service.ts (1)
1337-1399: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMove the RunLog read out of the per-user write lease.
hydrateJobViewFromRunLogat Line 1388 runs insidewithAccountWriteLease'swrite()callback, afterupdateJobRowhas already committed. This call exists only to enrich the returnedjobview; it does not affect the write's correctness. Holding the lease during this extra RPC round trip serializes other operations for the same user behind it unnecessarily.Return the updated record from
write()and hydrate after the lease is released.🔧 Proposed fix to hydrate after the lease is released
export async function runJobNow(input: { env: Env userId: string jobId: string callerContext?: McpCallerContext | null repoCheckPolicyOverride?: JobRepoCheckPolicy | null waitUntil?: (promise: Promise<unknown>) => void }) { - return await withAccountWriteLease({ + const result = await withAccountWriteLease({ db: input.env.APP_DB, stableUserId: input.userId, async write() { // ... unchanged up to updateJobRow ... - const job = await hydrateJobViewFromRunLog({ - env: input.env, - userId: input.userId, - job: toJobView(updated), - }) return { - job, + job: toJobView(updated), execution: outcome.execution, deletedAfterRun, } }, }) + const job = await hydrateJobViewFromRunLog({ + env: input.env, + userId: input.userId, + job: result.job, + }) + return { ...result, job } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/jobs/service.ts` around lines 1337 - 1399, Move the hydrateJobViewFromRunLog call out of the withAccountWriteLease write callback. Have write() return the updated job record and execution result, then invoke hydrateJobViewFromRunLog after withAccountWriteLease resolves so the RunLog read occurs after the lease is released while preserving the existing response fields and deletion status.packages/worker/src/run-records/service.node.test.ts (1)
26-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerify the activation seed path through this stub.
With
surface: 'job',status: 'success', andpackageId: 'pkg-a',prepareTerminalRunSideEffectSeedsreachesensureActivationStateSeeded, which callsrpc.isActivationInitialized()andrpc.importActivationState(). Add those RunLog RPC mocks and assert the seed-call expectation if the test should exercise seeding; otherwise silence the skipped seed warning whenRUN_LOG.get()only supports run lifecycle methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/run-records/service.node.test.ts` around lines 26 - 33, Update the RUN_LOG.get stub used by the terminal-run test to include isActivationInitialized and importActivationState RPC mocks. Configure the test inputs for the job success seed path and assert the expected activation seed calls from prepareTerminalRunSideEffectSeeds through ensureActivationStateSeeded, rather than allowing the stub to omit or warn about these methods.
🧹 Nitpick comments (8)
packages/worker/src/account/export.node.test.ts (1)
1653-1669: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: keep the
run_lognotes assertions in one test file.
packages/worker/src/account/user-owned-surfaces.node.test.tslines 101-113 assert the samerun_logsurface metadata and note substrings. Keep the surface-metadata assertions in the owning test file and keep the export test focused on export behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/account/export.node.test.ts` around lines 1653 - 1669, Remove the duplicated run_log metadata and notes assertions from the test containing “run_log surface notes document clearAll purging every RunLog table,” leaving those checks in user-owned-surfaces.node.test.ts. Keep this export test focused solely on export behavior.packages/worker/src/run-records/service.ts (1)
570-594: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: skip the seed probe after the first successful seed.
Every terminal finish with a
jobIdcallsgetJobRunObservabilitybeforefinishRun, and every qualifying success callsisActivationInitialized. Both remain one extra awaited DO RPC per terminal run after seeding has completed. Consider folding the seed check into the terminalfinishRunRPC, or caching the seeded state per isolate, to remove the extra round trip from the hot path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/run-records/service.ts` around lines 570 - 594, Optimize prepareTerminalRunSideEffectSeeds by avoiding repeated observability and activation seed probes after their first successful initialization. Reuse a per-isolate cache or fold the checks into the terminal finishRun flow, while preserving seeding for each previously uninitialized job or activation state.packages/worker/src/package-invocations/service.node.test.ts (1)
252-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe fake activation logic diverges from the Durable Object implementation.
RunLogBase.applySuccessfulPackageActivationIncrementreturns early when the globalpackage_activatedmilestone already exists, so it stops incrementingpackage_run_successesfor every package. This fake always increments and only guards the milestone inserts. A test that reaches activation and then finishes more invocations will observe counts that the real Durable Object never produces.Align the fake with the Durable Object, or add a comment that states the fake models only the pre-activation path.
♻️ Proposed alignment
if (!alreadyTerminal && runStatus === 'success') { + // Mirrors the DO: once `package_activated` exists, counting stops. + if (activationMilestones.has('package_activated')) return { + ledgerUpdated, + record: ledgerUpdated ? null : row ? clone(row) : null, + } const packageId =🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-invocations/service.node.test.ts` around lines 252 - 303, Update the fake activation logic in the test’s run-row persistence handler to match RunLogBase.applySuccessfulPackageActivationIncrement: return or skip all package_run_successes and milestone updates once the global package_activated milestone exists. Preserve the existing terminal-write and surface/package filtering behavior for pre-activation runs.packages/worker/src/run-records/continuity-and-retention.workers.test.ts (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth test files copy one
console.warnmock that discards every message. The mock returns for expected substrings and also returns for all other messages, so no unexpected warning reaches the console or fails a test.
packages/worker/src/run-records/continuity-and-retention.workers.test.ts#L28-L34: forward non-matching messages to the previous mock implementation, then move the helper into shared test support.packages/worker/src/run-records/dedicated-state.workers.test.ts#L40-L46: delete the duplicate and import the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/run-records/continuity-and-retention.workers.test.ts` around lines 28 - 34, Update packages/worker/src/run-records/continuity-and-retention.workers.test.ts lines 28-34 by moving silenceExpectedConsoleWarns into shared test support and forwarding non-matching warnings to the previous console.warn mock implementation. Update packages/worker/src/run-records/dedicated-state.workers.test.ts lines 40-46 by deleting its duplicate helper and importing the shared one.packages/worker/src/run-records/run-log-do.ts (1)
655-665: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer schema introspection to suppress the duplicate column error.
For a fresh Durable Object,
installedVersionisnull, so this path runsALTER TABLE job_run_observability ADD COLUMN legacy_seeded ...even thoughCREATE TABLEalready defines that column. UsePRAGMA table_info(job_run_observability)orsqlite_masterto check whether the column exists before running the alter, instead of relying on SQL duplicate-column errors as expected control flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/run-records/run-log-do.ts` around lines 655 - 665, Update the schema migration block in the run-log Durable Object to introspect job_run_observability with PRAGMA table_info or sqlite_master before adding legacy_seeded. Only execute the ALTER TABLE statement when the column is absent, and remove the try/catch that uses duplicate-column errors as expected control flow; preserve the existing installedVersion threshold.packages/worker/src/package-runtime/package-workflows.node.test.ts (1)
9-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an inline type specifier for the
service.tsimport.The static check reports
consistent-type-specifier-stylefor this top-level type-only import.♻️ Proposed fix
-import type { - WorkflowProjectionRecord, - WorkflowProjectionUpsertInput, -} from '`#worker/run-records/service.ts`' +import { + type WorkflowProjectionRecord, + type WorkflowProjectionUpsertInput, +} from '`#worker/run-records/service.ts`'The same warning applies to
packages/worker/src/package-runtime/package-workflows-cancel.node.test.tsLines 4-7 andpackages/worker/src/mcp/capabilities/durable-escalation.node.test.tsLines 10-13.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-runtime/package-workflows.node.test.ts` around lines 9 - 12, Update the type-only imports from run-records/service.ts in package-workflows.node.test.ts, package-workflows-cancel.node.test.ts, and durable-escalation.node.test.ts to use inline type specifiers for each imported symbol, preserving the existing imported types and import source.Source: Linters/SAST tools
packages/worker/src/mcp/capabilities/durable-escalation.ts (1)
96-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing D1-to-projection mapping instead of duplicating it.
This block re-implements the row mapping that
mapD1WorkflowRunRowandprojectionUpsertFromInspectionalready perform inpackages/worker/src/package-runtime/package-workflows.ts(Lines 649-705). The two copies also disagree on one rule:mapD1WorkflowRunRowthrows for an unknownsource_type, while Line 118 here coerces any unknown value to'inline'.Export a narrow importer from
package-workflows.ts(for exampleimportCreatingWorkflowRunFromD1) and call it here. That keeps one mapping and one validation rule for the legacyworkflow_runsshape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/capabilities/durable-escalation.ts` around lines 96 - 138, Replace the duplicated row-to-projection mapping in importCreatingD1RowIfPresent with a narrow exported importer from package-workflows.ts, such as importCreatingWorkflowRunFromD1, and invoke it after fetching the D1 row. Reuse mapD1WorkflowRunRow/projectionUpsertFromInspection so unknown source_type values are validated consistently instead of coerced to inline.packages/worker/src/package-runtime/package-workflows-cancel.node.test.ts (1)
17-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared RunLog projection fake into test support.
This hoisted store, the
vi.mock('#worker/run-records/service.ts')forwarder, andcreateWaitUntilFlusherare near-identical copies of Lines 37-366 inpackages/worker/src/package-runtime/package-workflows.node.test.ts. Two more partial copies exist inpackages/worker/src/mcp/capabilities/durable-escalation.node.test.ts(Lines 19-166) andpackages/worker/src/mcp/run-kody-registry.node.test.ts(Lines 37-245).The copies already differ. The
run-kody-registry.node.test.tscopy omits the terminal-status stickiness rule that this copy applies at Lines 90-101. A fake that models RunLog write ordering is correctness-critical, so drift between copies weakens every test that depends on it.Move the store and the forwarder into a shared helper under
packages/worker/src/test-support/, and let each test file import it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-runtime/package-workflows-cancel.node.test.ts` around lines 17 - 322, Extract the shared RunLog projection fake currently defined by runRecordMocks, the vi.mock('`#worker/run-records/service.ts`') forwarder, and createWaitUntilFlusher into a helper under test-support. Preserve the complete behavior here, including updatedAt ordering and terminal-status stickiness, then update package-workflows.node.test.ts, durable-escalation.node.test.ts, and run-kody-registry.node.test.ts to import and reuse that helper instead of maintaining local copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/architecture/data-storage.md`:
- Around line 251-261: The storage documentation still attributes run counters
and terminal outcomes to D1 jobs. In
docs/contributing/architecture/data-storage.md lines 251-261, revise the Storage
split list to limit D1 jobs to schedule fields, last_run_at, source pointers,
and storage_id; in docs/contributing/architecture/run-records.md lines 243-247,
update the Entity state row and paragraph so last-run outcomes and counters are
attributed to RunLog job_run_observability.
In `@packages/worker/src/jobs/job-run-observability-hydrate.ts`:
- Around line 13-28: Distinguish a missing observability record from a RunLog
lookup failure in getJobRunObservability and getJobRunObservabilityBatch, rather
than representing both as null or an empty result. Propagate that availability
state to applyJobRunObservabilityToJobView so a RunLog outage produces an
explicit “status unknown” outcome instead of retaining or implying stale success
data, while preserving normal job values when no record exists.
In `@packages/worker/src/package-runtime/package-workflows.ts`:
- Around line 861-887: Update importActiveD1WorkflowRuns and
importRecentD1WorkflowRuns to start all importWorkflowInspectionIntoRunLog calls
without awaiting each iteration, then await their combined completion so per-row
RPCs overlap while preserving completion before either function returns.
In `@packages/worker/src/run-records/run-log-do.ts`:
- Around line 1406-1412: The production method
applySuccessfulPackageActivationIncrement currently skips package_run_successes
after the global package_activated milestone; document this counter contract in
its doc comment, or move the milestone check after incrementing if counters are
intended as per-package totals. In
packages/worker/src/package-invocations/service.node.test.ts lines 252-303, make
the fake apply the same short-circuit behavior or explicitly document that it
models only the pre-activation path.
In `@packages/worker/src/run-records/service.ts`:
- Around line 438-442: Update isMissingD1RelationError to recognize D1 “no such
column” errors in addition to “no such table,” so readJobRunObservabilityFromD1
treats schemas lacking legacy observability columns as a successful empty
snapshot, sets legacySeeded, and avoids retrying the query on subsequent
terminal job finishes.
---
Outside diff comments:
In `@packages/worker/src/jobs/service.ts`:
- Around line 1337-1399: Move the hydrateJobViewFromRunLog call out of the
withAccountWriteLease write callback. Have write() return the updated job record
and execution result, then invoke hydrateJobViewFromRunLog after
withAccountWriteLease resolves so the RunLog read occurs after the lease is
released while preserving the existing response fields and deletion status.
In `@packages/worker/src/run-records/service.node.test.ts`:
- Around line 26-33: Update the RUN_LOG.get stub used by the terminal-run test
to include isActivationInitialized and importActivationState RPC mocks.
Configure the test inputs for the job success seed path and assert the expected
activation seed calls from prepareTerminalRunSideEffectSeeds through
ensureActivationStateSeeded, rather than allowing the stub to omit or warn about
these methods.
---
Nitpick comments:
In `@packages/worker/src/account/export.node.test.ts`:
- Around line 1653-1669: Remove the duplicated run_log metadata and notes
assertions from the test containing “run_log surface notes document clearAll
purging every RunLog table,” leaving those checks in
user-owned-surfaces.node.test.ts. Keep this export test focused solely on export
behavior.
In `@packages/worker/src/mcp/capabilities/durable-escalation.ts`:
- Around line 96-138: Replace the duplicated row-to-projection mapping in
importCreatingD1RowIfPresent with a narrow exported importer from
package-workflows.ts, such as importCreatingWorkflowRunFromD1, and invoke it
after fetching the D1 row. Reuse
mapD1WorkflowRunRow/projectionUpsertFromInspection so unknown source_type values
are validated consistently instead of coerced to inline.
In `@packages/worker/src/package-invocations/service.node.test.ts`:
- Around line 252-303: Update the fake activation logic in the test’s run-row
persistence handler to match
RunLogBase.applySuccessfulPackageActivationIncrement: return or skip all
package_run_successes and milestone updates once the global package_activated
milestone exists. Preserve the existing terminal-write and surface/package
filtering behavior for pre-activation runs.
In `@packages/worker/src/package-runtime/package-workflows-cancel.node.test.ts`:
- Around line 17-322: Extract the shared RunLog projection fake currently
defined by runRecordMocks, the vi.mock('`#worker/run-records/service.ts`')
forwarder, and createWaitUntilFlusher into a helper under test-support. Preserve
the complete behavior here, including updatedAt ordering and terminal-status
stickiness, then update package-workflows.node.test.ts,
durable-escalation.node.test.ts, and run-kody-registry.node.test.ts to import
and reuse that helper instead of maintaining local copies.
In `@packages/worker/src/package-runtime/package-workflows.node.test.ts`:
- Around line 9-12: Update the type-only imports from run-records/service.ts in
package-workflows.node.test.ts, package-workflows-cancel.node.test.ts, and
durable-escalation.node.test.ts to use inline type specifiers for each imported
symbol, preserving the existing imported types and import source.
In `@packages/worker/src/run-records/continuity-and-retention.workers.test.ts`:
- Around line 28-34: Update
packages/worker/src/run-records/continuity-and-retention.workers.test.ts lines
28-34 by moving silenceExpectedConsoleWarns into shared test support and
forwarding non-matching warnings to the previous console.warn mock
implementation. Update
packages/worker/src/run-records/dedicated-state.workers.test.ts lines 40-46 by
deleting its duplicate helper and importing the shared one.
In `@packages/worker/src/run-records/run-log-do.ts`:
- Around line 655-665: Update the schema migration block in the run-log Durable
Object to introspect job_run_observability with PRAGMA table_info or
sqlite_master before adding legacy_seeded. Only execute the ALTER TABLE
statement when the column is absent, and remove the try/catch that uses
duplicate-column errors as expected control flow; preserve the existing
installedVersion threshold.
In `@packages/worker/src/run-records/service.ts`:
- Around line 570-594: Optimize prepareTerminalRunSideEffectSeeds by avoiding
repeated observability and activation seed probes after their first successful
initialization. Reuse a per-isolate cache or fold the checks into the terminal
finishRun flow, while preserving seeding for each previously uninitialized job
or activation state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0787500-5537-4118-9648-d320ece1f9db
📒 Files selected for processing (41)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/run-records.mdpackages/worker/src/account/export.node.test.tspackages/worker/src/account/export.tspackages/worker/src/account/user-owned-surfaces.node.test.tspackages/worker/src/account/user-owned-surfaces.tspackages/worker/src/jobs/execution-safety.node.test.tspackages/worker/src/jobs/inspect.node.test.tspackages/worker/src/jobs/inspect.tspackages/worker/src/jobs/job-retention.node.test.tspackages/worker/src/jobs/job-retention.tspackages/worker/src/jobs/job-run-observability-hydrate.node.test.tspackages/worker/src/jobs/job-run-observability-hydrate.tspackages/worker/src/jobs/job-schedule-watchdog.node.test.tspackages/worker/src/jobs/job-schedule-watchdog.tspackages/worker/src/jobs/process-due-jobs.node.test.tspackages/worker/src/jobs/process-due-jobs.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/repo.workers.test.tspackages/worker/src/jobs/run-due-jobs-claim-fence.node.test.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/durable-escalation.node.test.tspackages/worker/src/mcp/capabilities/durable-escalation.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-runtime/package-workflows-cancel.node.test.tspackages/worker/src/package-runtime/package-workflows.node.test.tspackages/worker/src/package-runtime/package-workflows.tspackages/worker/src/run-records/continuity-and-retention.workers.test.tspackages/worker/src/run-records/dedicated-state.workers.test.tspackages/worker/src/run-records/job-run-observability.tspackages/worker/src/run-records/package-activation-state.tspackages/worker/src/run-records/run-log-do.tspackages/worker/src/run-records/run-records.workers.test.tspackages/worker/src/run-records/service.node.test.tspackages/worker/src/run-records/service.tspackages/worker/src/run-records/types.tspackages/worker/src/run-records/workflow-projection.tspackages/worker/src/usage/activation.node.test.tspackages/worker/src/usage/activation.ts
| export function applyJobRunObservabilityToJobView( | ||
| job: JobView, | ||
| observability: JobRunObservabilityRecord | null | undefined, | ||
| ): JobView { | ||
| if (!observability) return job | ||
| return { | ||
| ...job, | ||
| lastRunAt: observability.lastRunAt ?? job.lastRunAt, | ||
| lastRunStatus: observability.lastRunStatus ?? job.lastRunStatus, | ||
| lastRunError: observability.lastRunError ?? undefined, | ||
| lastDurationMs: observability.lastDurationMs ?? undefined, | ||
| runCount: observability.runCount, | ||
| successCount: observability.successCount, | ||
| errorCount: observability.errorCount, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
RunLog outage silently blanks job error/duration/count data.
applyJobRunObservabilityToJobView returns job unchanged whenever observability is null or undefined. getJobRunObservability and getJobRunObservabilityBatch return null or an empty array both when no record exists and when the RunLog RPC call fails or the binding is unavailable. Since D1 no longer stores a fresh copy of lastRunError, lastDurationMs, or the run counters (this cohort freezes them at their prior D1 values), a RunLog outage during a job that actually failed results in a job view with no error message and stale counters, and the caller has no way to distinguish "no error occurred" from "RunLog was unreachable".
Consider surfacing availability distinctly, so consumers can show a "status unknown" state instead of implying success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/jobs/job-run-observability-hydrate.ts` around lines 13 -
28, Distinguish a missing observability record from a RunLog lookup failure in
getJobRunObservability and getJobRunObservabilityBatch, rather than representing
both as null or an empty result. Propagate that availability state to
applyJobRunObservabilityToJobView so a RunLog outage produces an explicit
“status unknown” outcome instead of retaining or implying stale success data,
while preserving normal job values when no record exists.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| ? serializeCallerContext(activeCallerContext) | ||
| : row.callerContextJson, | ||
| }) | ||
| return { |
There was a problem hiding this comment.
Update job skips RunLog overlay
Medium Severity
updateJob still returns a toJobView built only from the D1 scheduling row. After this change, terminal counters, last-run error, and duration live in RunLog job_run_observability, and applyExecutionOutcome no longer updates those fields on D1. job_update (and any caller using the mutation return value) can show zero counts and missing errors even when runs succeeded, while job_list, job_get, and runJobNow already hydrate from RunLog.
Reviewed by Cursor Bugbot for commit 86f066b. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2fdca12. Configure here.
| env: input.env, | ||
| userId: input.userId, | ||
| job: toJobView(updated), | ||
| }) |
There was a problem hiding this comment.
Run now stale observability
Medium Severity
After a manual run, runJobNow hydrates the returned JobView from RunLog while JobManager.runNow still passes waitUntil, so finishRunRecord can persist terminal job_run_observability asynchronously. D1 no longer updates counters or error/duration in applyExecutionOutcome, so the response can show the previous run’s counts, error, duration, or status until the deferred finish completes.
Reviewed by Cursor Bugbot for commit 2fdca12. Configure here.
3 participants
Summary
Validation
CI=1 npm run validate— passed locally on2fdca129(1,706 unit/Workers tests plus E2E/MCP/static/typecheck/build/docs/migrations)System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@915db38a· Head:2fdca129Classification: extends — RunLog gains authoritative dedicated-state contracts and workflow/job/activation readers are rewired with expand-phase continuity.
Primitives touched
run-recordsworkflowsjobsusage-meteringaccount-exportcapability-registrySystem map
Workflow/job terminal events flow into the per-user RunLog; bounded legacy reads seed pre-deploy D1 state before authoritative decisions.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
RUN_LOG.idFromName(userId); dedicated tables omituser_idbecause the Durable Object is user-scoped.clearAlland account export cover every dedicated table.Conductor report
STATUS: done
What shipped: additive per-user RunLog workflow projections, job observability, package success counters, and activation milestones; bounded legacy-state cutover; atomic entitlement reservations and terminal transitions; export/deletion/retention guardrails; updated architecture docs.
Risk self-assessment: high —
concurrent_workflowsenforcement semantics now atomically reserve against RunLog, so the PR is green and ready-for-review but intentionally not self-merged.Merged/deployed: no / no. PR: #1114 · CI: https://github.com/kentcdodds/kody/actions/runs/30675733325
Scope spill: meter-do should move shared
concurrent_workflowsusage readers inentitlements/service.tsto the exposed RunLog count; reporting-off-d1 must replace legacy workflow/job/activation aggregates inapp/admin-insights-data.tsbefore compatibility stores retire.Summary by CodeRabbit